Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add HaarGraph #408

Merged
merged 1 commit into from
Mar 11, 2021
Merged

Add HaarGraph #408

merged 1 commit into from
Mar 11, 2021

Conversation

finnbuck
Copy link
Contributor

@finnbuck finnbuck commented Feb 13, 2021

Add global function for generating a Haar Graph based on a single parameter. @flsmith

gap/examples.gi Outdated Show resolved Hide resolved
@olexandr-konovalov olexandr-konovalov mentioned this pull request Feb 20, 2021
gap/examples.gi Outdated Show resolved Hide resolved
@marinaanagno marinaanagno self-requested a review February 24, 2021 14:29
@samanthaharper
Copy link
Contributor

I think documentation would be really helpful to reviewing this graph, since I'm not sure what you're using to construct the Haar graph.

Copy link
Contributor

@marinaanagno marinaanagno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have consulted Wolfram Alpha to see what this graph is about, and it seems to agree with the definition. However, if we proceed according to what was said in the meeting today, documentation should be added and also this function should be declared as a constructor and operations that return the final mutable and immutable version of the digraph should be added :)

@finnbuck
Copy link
Contributor Author

Ok sounds good! I'll have a look at some examples and try to fix the documentation and function declaration...

@james-d-mitchell
Copy link
Member

@finnbuck looks ok to me, I think you need to write some doc (as per the other comments) and also to follow the same "pattern" as in the Folkman PR #386. Other than that: good stuff!

@james-d-mitchell james-d-mitchell added the new-feature A label for new features. label Mar 1, 2021
@finnbuck
Copy link
Contributor Author

finnbuck commented Mar 3, 2021

DIGRAPHS_BlistNumber([IsInt], [IsInt]) is taken from a GAP Semigroups function of the same name and is not meant to be user facing here, so I'm assuming it's meant to stay a global function.

@wilfwilson
Copy link
Collaborator

wilfwilson commented Mar 3, 2021

Closing and reopening to (hopefully) re-trigger the CI.

Note, the Azure Pipelines jobs were not starting...

##[error]The agent did not connect within the alloted time of 45 minute(s).
,##[error]The request: 2903 was abandoned due to an infrastructure failure. Notification of assignment to an agent was never received.

@wilfwilson wilfwilson closed this Mar 3, 2021
@wilfwilson wilfwilson reopened this Mar 3, 2021
flsmith
flsmith previously requested changes Mar 3, 2021
doc/examples.xml Outdated Show resolved Hide resolved
doc/examples.xml Outdated Show resolved Hide resolved
doc/examples.xml Outdated Show resolved Hide resolved
gap/examples.gi Show resolved Hide resolved
gap/examples.gi Outdated Show resolved Hide resolved
gap/examples.gi Outdated Show resolved Hide resolved
gap/examples.gi Outdated Show resolved Hide resolved
gap/examples.gi Outdated Show resolved Hide resolved
tst/standard/examples.tst Outdated Show resolved Hide resolved
gap/examples.gi Outdated Show resolved Hide resolved
@flsmith flsmith mentioned this pull request Mar 3, 2021
gap/examples.gi Outdated Show resolved Hide resolved
@digraphs digraphs deleted a comment from codecov bot Mar 11, 2021
Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is looking nice! Thanks. I'll commit these changes, and then squash, rebase and onto master, and push... and then merge if all seems well 🙂

gap/examples.gi Show resolved Hide resolved
@wilfwilson
Copy link
Collaborator

@finnbuck I'll note that the documentation you wrote wasn't actually linked into the manual - see for reference: https://github.com/digraphs/Digraphs/pull/425/files#r592297697

But I'll do that for you now.

doc/examples.xml Outdated Show resolved Hide resolved
@wilfwilson wilfwilson dismissed flsmith’s stale review March 11, 2021 16:34

Changes were addressed!

Copy link
Collaborator

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully it's good to go!

@wilfwilson wilfwilson changed the title Haar Add HaarGraph Mar 11, 2021
@wilfwilson wilfwilson merged commit e7381a8 into digraphs:master Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature A label for new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants