Allow Core pushforward to accept more ring maps#4435
Conversation
f4c93aa to
1f7ee01
Compare
1f7ee01 to
8a51940
Compare
| phiS^-1 mapback selectInSubring(if numgensR > 0 then 1 else 0, generators g)) | ||
|
|
||
| -- repair homogeneity if we had it to begin with. | ||
| if isHgs then result = try map(target result, , result) else result; |
There was a problem hiding this comment.
In my experience doing this fakes homogeneity rather than repairing it, possibly giving a false impression by covering up errors or mistakes.
There was a problem hiding this comment.
By "fakes homogeneity" do you mean that the result is not homogeneous even in cases where isHomogeneous may say that it is?
Alternatively maybe you mean that while the result may be homogeneous it is not graded in a way that allows for a homogenous bijection with m?
There was a problem hiding this comment.
isHomogeneous fails when either the source or target of the matrix are not correct. Here you're assuming that the target is correct and updating the source accordingly. But if there was a mistake that made the source incorrect, how do you know the target was correct? If the target was incorrect, now the source is set incorrectly based on that too, but in a way that everything looks homogeneous.
There was a problem hiding this comment.
Ack that makes perfect sense. In that case I think it makes sense to just revert the change calling map and return result as is. This leaves it up to the caller of this method to know what the right source / target are and avoids giving a potentially misleading impression that the map returned is the right graded presentation for the push forward module being computed.
Does this sound right to you?
edit: the clarification I am looking for here is that there is no way to know what the "right" source and target are so no attempt should be made to make the result homogeneous.
There was a problem hiding this comment.
Oh no there is definitely a right source and target that should be set by the computation. My point is that if we get one or both wrong, we should hope to find out that something went wrong because the result is not homogeneous. Fixing homogeneity this way covers up potential mistakes.
There was a problem hiding this comment.
Ok I understand. I'll think more about the right way to preserve homogeneity.
|
OK the build failure is from generating examples and this has put my finger on the dependence between symmetricPower and the degree manipulations in the pushforward package. IMO ideally the symmetricPower context would be able to set up the degrees on the result of pushforward and not require injecting this specific degreemap into the push-forward logic. I will see what that might look like. |
|
@mahrud following your nudging, I landed on an approach that follows the intention of the previous impl - setting a custom degree map on the projection I |
| -- d' is the element of degreeGroup target f corresponding to d | ||
| d' := matrix transpose {d}; | ||
| -- get coefficients for d' against generators for G ~ D. | ||
| -- this produces mild nonsense if d' is not in D but that doesn't matter |
There was a problem hiding this comment.
I considered also checking explicitly whether d' % D == 0 and returning {0, ..., 0} in that case. It is not obvious how to interpret the return value of this function in cases when the argument is not in the image but we only ever apply the resulting mapback to elements in image f so I could not convince myself that it mattered.
|
@mahrud you mentioned that there was an example with negative grading that you found did not work well with these changes. Can you reproduce it and share the example and your expectations for it? |
When computing the push-forward of a module
Malong a ring mapf, the core pushforward.m2 includes a check enforcing the invariant that a certain degree map used in it's computations is a section of the degree map ofsource falongf. This ends up placing a restriction on which ring mapsfcan be used in this calculation that is I think more restrictive than it has to be.There is a comment in the code that the degree map used is somewhat arbitrary and chosen just to ensure a nice property of the symmetricPower functor. I was not able to identify how this pushforward functionality interacts with the symmetricPower method though.
This PR removes the idiosyncratic degree map and the block maintaining the invariant. My understanding is that the degree map used ends up modifying the result just in terms of the shifts of the free modules in the resulting presentation. To improve the behavior of this method in new cases a final regrading is done (by omitting the source module in a call to
map) in the case when the inputs were homogeneous.Previously the test case that is added here would fail on computing the pushforward module:
@mikestillman I think you are the original author of the
pushNonLinearmethod. Do you have any context you can provide on the necessity of the check that I am proposing to remove here? To my understanding this algorithm is quite happy to compute the correct result in more generality and the check is not needed but I would love to understand more what this was trying to guard against.