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

Improve error handling for Image, Images, PreImage, PreImages #3454

Merged

Conversation

ssiccha
Copy link
Contributor

@ssiccha ssiccha commented May 15, 2019

..., PreImage, and PreImages.

If one of these functions figures out that the second argument is not an
element or subset of the Range or the Source, it now tells the user.

@ssiccha ssiccha added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: library release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels May 15, 2019
@coveralls
Copy link

coveralls commented May 15, 2019

Coverage Status

Coverage increased (+0.007%) to 85.266% when pulling b4ab5d0 on ssiccha:ss/improve-error-handling-image-preimage into 597c793 on gap-system:master.

@codecov
Copy link

codecov bot commented May 17, 2019

Codecov Report

Merging #3454 into master will increase coverage by 4.88%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3454      +/-   ##
==========================================
+ Coverage   80.47%   85.35%   +4.88%     
==========================================
  Files         696      699       +3     
  Lines      343497   346557    +3060     
==========================================
+ Hits       276413   295797   +19384     
+ Misses      67084    50760   -16324
Impacted Files Coverage Δ
lib/mapping.gi 90.98% <100%> (+1.82%) ⬆️
hpcgap/lib/mapping.gi 90.41% <100%> (+1.21%) ⬆️
lib/gprdpc.gi 45.74% <0%> (-42.2%) ⬇️
src/julia_gc.c 77.4% <0%> (-5.33%) ⬇️
lib/domain.gi 87.69% <0%> (-3.04%) ⬇️
lib/gpprmsya.gi 78.29% <0%> (-2.69%) ⬇️
src/listfunc.c 94.7% <0%> (-1.02%) ⬇️
lib/csetgrp.gi 74.19% <0%> (-0.68%) ⬇️
lib/ghompcgs.gi 91.35% <0%> (-0.67%) ⬇️
src/records.c 87.43% <0%> (-0.66%) ⬇️
... and 192 more

@ChrisJefferson
Copy link
Contributor

Could you write tests to hit all these new cases, just to sanity check them?

@fingolfin fingolfin changed the title lib: improve error handling for Image, Images, ... Improve error handling for Image, Images, PreImage, PreImages May 17, 2019
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

I am not convinced this is correct, see my inline comment. I guess my claim on what worked before and now doesn't work should be verified. Also, as @ChrisJefferson suggested, tests would be really nice.

lib/mapping.gi Outdated Show resolved Hide resolved
@ssiccha
Copy link
Contributor Author

ssiccha commented May 17, 2019

I'll definitely set up some tests as @ChrisJefferson suggested and have a closer look at your inline comment @fingolfin.

@ssiccha ssiccha force-pushed the ss/improve-error-handling-image-preimage branch from 7fa0e7e to afa8ae0 Compare May 21, 2019 16:59
@ssiccha
Copy link
Contributor Author

ssiccha commented May 22, 2019

I'm updating the PR right now. I have added tests and deleted the

      else
          ErrorNoReturn( "<coll> must be a domain, a set, ",
                         "or a homogeneous list" );

lines. This "else" branch can't be executed since the function compares the families of the source and of the argument elm. The source must be a domain and thus will always have a different family in the situation that elm is not a homogeneous list.

@ssiccha ssiccha force-pushed the ss/improve-error-handling-image-preimage branch 2 times, most recently from bb96e86 to 8064400 Compare May 22, 2019 10:47
@ssiccha
Copy link
Contributor Author

ssiccha commented May 22, 2019

PR is updated.

@ssiccha ssiccha force-pushed the ss/improve-error-handling-image-preimage branch 2 times, most recently from 9e63abd to 67fb345 Compare May 22, 2019 16:28
@ssiccha
Copy link
Contributor Author

ssiccha commented May 22, 2019

And again some HPCGAP tests broke. Let's see whether it works now.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

In principle this seem to be on a good way, just some minor nitpicking

hpcgap/lib/mapping.gi Outdated Show resolved Hide resolved
hpcgap/lib/mapping.gi Outdated Show resolved Hide resolved
hpcgap/lib/mapping.gi Outdated Show resolved Hide resolved
hpcgap/lib/mapping.gi Outdated Show resolved Hide resolved
@ssiccha ssiccha force-pushed the ss/improve-error-handling-image-preimage branch from 67fb345 to 1e746b2 Compare May 25, 2019 13:30
@ssiccha
Copy link
Contributor Author

ssiccha commented May 25, 2019

I've addressed your remarks.

@ssiccha ssiccha force-pushed the ss/improve-error-handling-image-preimage branch from 1e746b2 to 070dfae Compare May 27, 2019 08:37
Copy link
Member

@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 have some requests concerning the error messages, particular about the use of the name <coll>.

hpcgap/lib/mapping.gi Outdated Show resolved Hide resolved
hpcgap/lib/mapping.gi Show resolved Hide resolved
hpcgap/lib/mapping.gi Outdated Show resolved Hide resolved
hpcgap/lib/mapping.gi Outdated Show resolved Hide resolved
hpcgap/lib/mapping.gi Outdated Show resolved Hide resolved
@ssiccha
Copy link
Contributor Author

ssiccha commented Jun 24, 2019

I adopted the suggested improvements.

I put the changes into a separate commit so you can see more easily what I've changed. I'll turn the new commits into a single one after the PR is accepted.

@ssiccha ssiccha force-pushed the ss/improve-error-handling-image-preimage branch from 1764792 to 21d7375 Compare June 24, 2019 11:42
@ssiccha
Copy link
Contributor Author

ssiccha commented Jun 24, 2019

.. and I rebased onto master.

Copy link
Member

@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.

Thanks for your changes.

..., `Images`, `PreImage`, and `PreImages`.

If one of these functions figures out that the second argument is not an
element or subset of the Range or the Source, it now tells the user.
The same changes are replicated to `hpcgap/lib/mapping.gi`.

Improves the structuring of the `mapping.tst` file. Adds tests for the
new error handling.

Also adds some general tests for `Image`, `Images`, `PreImage`,
`PreImages`.
@ssiccha ssiccha force-pushed the ss/improve-error-handling-image-preimage branch from 21d7375 to b4ab5d0 Compare June 24, 2019 12:24
@ssiccha
Copy link
Contributor Author

ssiccha commented Jun 24, 2019

Squashed.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thank you!

@fingolfin fingolfin merged commit b7c6eb2 into gap-system:master Jun 27, 2019
@ssiccha ssiccha deleted the ss/improve-error-handling-image-preimage branch August 7, 2019 08:47
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 21, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: added PRs introducing changes that have since been mentioned in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants