Conversation
1c4106b to
8edf39e
Compare
- remove code duplication for detecting morphisms that map generators to generators - expand the note in the `eliminate` docstring to point to `preimage` - add an explicit example to the docstring
HechtiDerLachs
left a comment
There was a problem hiding this comment.
Thank you for the cleanup!
This PR streamlines the use of _maps_variables_to_variables with the previous ad hoc implementations of the same functionality in the constructor of maps.
| if check_for_mapping_of_vars | ||
| result.variable_indices = __maps_variables_to_variables(img_gens, | ||
| codomain) |
There was a problem hiding this comment.
In the previous implementation this field being unassigned was the indicator that the map does not take variables into variables. Now it seems that the field is assigned by default. But I suppose you deliberately changed the overall concept here and took that into account, right?
Either way: I was just a bit puzzled that this field is assigned only if a specific kwarg is set to true.
There was a problem hiding this comment.
- Yes, I changed the concept so that there is now only one way to ask whether variables map to variables. Before there were two: Either call
_maps_variables_to_variablesor checkisdefined(f, ...). - The part with always setting the field is on purpose. Repeatedly constructing
Int[]is expensive. Now one should never queryisdefined(f, :)directly, but just ask whether_maps_...is true. - The special kwarg was added by you. I am happy to remove it, since it is never used and cannot be used legally using
hom, see Speed up polynomial mappings #4124 (comment).
There was a problem hiding this comment.
I see. I think at the time I wanted to leave some backdoor open to avoid the check. That's why I introduced the kwarg, but did not yet forward it to the user-facing constructors. If it does not conflict with your implementation, then I would say: let's just keep it.
Not relying on isdefined anymore makes sense. This was a bit opaque, anyway.
There was a problem hiding this comment.
OK, let's keep keeping it. Thanks for the review.
fieker
left a comment
There was a problem hiding this comment.
if this helps, I'm happy. Avoiding GB is always good
eliminate using a proper subring
- remove code duplication for detecting morphisms that map generators to generators - expand the note in the `eliminate` docstring to point to `preimage` - add an explicit example to the docstring
eliminatedocstring to point topreimageCC: @fieker
Probably closes #2590.
The docstring is expanded by adding the following example: