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

Implement iteration over GAP iterators #967

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

fingolfin
Copy link
Member

No description provided.

Copy link
Member

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

Logically, this extension is fine:
Up to now, a new GAP iterator gets created when Julia iterates over a domain. Now a new iterator (a shallow copy) gets created when Julia iterates over a GAP iterator.

Perhaps the differences between iterating in GAP and in Julia can be documented (where is a good place for that?), in order to avoid confusion:

  • Iterating in GAP over an iterator changes the iterator, iterating in Julia doesn't.
  • Iterating in GAP over a list with holes skips the holes, iterating in Julia over a GAP list with holes also iterates over the holes (well, the holes before the end of the list) by returning nothing for each hole.

@fingolfin
Copy link
Member Author

Perhaps the differences between iterating in GAP and in Julia can be documented (where is a good place for that?), in order to avoid confusion:

  • Iterating in GAP over an iterator changes the iterator, iterating in Julia doesn't.

I could of course change it so that Julia iteration also "eats up" the GAP iterator. It just seemed less useful and would make it behave less like a Julia iterator.

That said, how would this cause trouble for anyone coming from GAP? Maybe people would copy iterators too often thinking they need to, but that's note a major issue.

  • Iterating in GAP over a list with holes skips the holes, iterating in Julia over a GAP list with holes also iterates over the holes (well, the holes before the end of the list) by returning nothing for each hole.

Ah, good point. Perhaps we should skip holes? I don't think it was a very intentional decision to not skip them.

@fingolfin fingolfin closed this Feb 16, 2024
@fingolfin fingolfin reopened this Feb 16, 2024
Copy link

codecov bot commented Feb 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.85%. Comparing base (ae2997e) to head (e58a6e9).
Report is 80 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #967      +/-   ##
==========================================
+ Coverage   75.80%   75.85%   +0.05%     
==========================================
  Files          51       51              
  Lines        4187     4188       +1     
==========================================
+ Hits         3174     3177       +3     
+ Misses       1013     1011       -2     
Files with missing lines Coverage Δ
src/adapter.jl 72.38% <100.00%> (+1.09%) ⬆️
src/constructors.jl 98.71% <100.00%> (ø)
src/wrappers.jl 98.57% <100.00%> (+0.02%) ⬆️

... and 1 file with indirect coverage changes

@ThomasBreuer
Copy link
Member

@fingolfin The message #I The Julia package 'Downloads' cannot be loaded. appears in the test logs,
for a failed test as well as for a successful test.
I do not understand why this message appears: First, the package should be loadable since it is listed as a dependency. Second, the only function that shows this message is the GAP function JuliaImportPackage from the JuliaInterface package. But who calls this function with the argument "Downloads"?

@fingolfin fingolfin closed this Sep 30, 2024
@fingolfin fingolfin reopened this Sep 30, 2024
@fingolfin fingolfin enabled auto-merge (squash) September 30, 2024 08:49
@fingolfin fingolfin merged commit 5ba3c3f into oscar-system:master Sep 30, 2024
58 of 71 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants