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

Fix ordering in (co)domain number accessors for schemas #521

Merged
merged 3 commits into from
Oct 13, 2021

Conversation

bosonbaas
Copy link
Member

This PR provides a fix for Issue #520. The error was that the adom_num and acodom_num functions assumed a certain ordering on the codoms field. This is now explicitly aligned to the order of the attrs field.

@epatters
Copy link
Member

epatters commented Oct 12, 2021

Thank you, Andrew, for reporting the bug and finding a fix! Following the struct acsets refector, I am less familiar with this part of the codebase than I used to be, but I thought that doms and codoms were named tuples and so I'm surprised that they don't have the correct order.

It also makes me wonder: do the functions dom_nums and codom_nums also have this subtle bug?

@olynch, it would be great if you could carefully review this issue.

@epatters epatters added the bug label Oct 12, 2021
@epatters epatters requested review from olynch and removed request for mehalter October 12, 2021 03:39
@github-actions
Copy link
Contributor

github-actions bot commented Oct 12, 2021

Review Checklist

Does this PR follow the development guidelines? Following is a partial checklist:

Tests

  • New features and bug fixes have unit tests
  • New modules have tests that are ultimately called by the test runner (test/runtests.jl)
  • Existing tests have not been deleted
  • Code coverage >= 90% or reduction justified in PR

Documentation

  • All exported functions, types, and constants have docstrings, written in complete sentences
  • Citations are given for any constructions, algorithms, or code drawn from external sources

Other

  • Style guidelines are followed, including indent width 2
  • Changes breaking backwards compatibility have been approved

@AlgebraicJulia AlgebraicJulia deleted a comment from github-actions bot Oct 12, 2021
@epatters epatters changed the title Fix type error with bundling legs Fix ordering in (co)domain number accessors for schemas Oct 13, 2021
Copy link
Member

@olynch olynch left a comment

Choose a reason for hiding this comment

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

Yes, I see the bug and the fix results in cleaner code, you can go ahead and merge this.

@bosonbaas
Copy link
Member Author

Should we also add the change to the dom_nums codom_nums functions above those?

@epatters
Copy link
Member

Let's go ahead and change those functions too. If nothing else, the code will be shorter.

@bosonbaas
Copy link
Member Author

Got those changes added, and it looks like tests are passing!

@epatters
Copy link
Member

Great, thanks Andrew!

@epatters epatters merged commit 9ba6380 into AlgebraicJulia:master Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants