Ensuring PALP fails consistently without returning misleading data

[rashadalsharpini]

Description

it addresses issue #41400
where PALP fails and in the first call for n_points it fails (good)
second time it returns the number of vertices (wrong)

Changes

• delayed the assignment of self._points
• added try catch, to make behavior more consistent

tests

• passed all related tests
• recreated the test from addressed issue and it worked

[rashadalsharpini]

@fchapoton so what do you think does this fix it?

[orlitzky]

The design of this method is a little weird so I may be overlooking some complication, but I think it would be much simpler if we did the cache check at the beginning of the method, for example:

if self._points is not None:
    return self._points(*args, **kwds)

# rest of the function
M = self.lattice()
nv = self.n_vertices()
points = self._vertices
...
if len(points) > nv:
    points = PointCollection(points, M)
self._points = points
return self._points(*args, **kwds)

From what I understand PALP already raises an exception when it fails. So long as the premature self._points = self._vertices assignment is not made at the beginning of the function, we will not get the wrong answer on the second call.

[rashadalsharpini]

your approach is valid but i kind went this way to only cache on success so i completely avoid any garbage value that might happen, so if everything goes smoothly then i cache otherwise it's just empty

[orlitzky]

Sure, that is the goal. In my example, self._points only appears two places:

1. In the first two lines of the method; if a value is cached, we immediately return and exit.
2. In the last two lines of the function; to cache the value that we have just (successfully) computed.

When the class is initiated, self._points is None, so the only way for it to become not None is for the method to succeed. There are of course many ways to do it, but in my example the "is the value cached?" cases are handled immediately, whereas in the original code they span the entire length of the method -- at the very end you have to remember that you are exiting an "if" statement that started ~30 lines ago.

Putting that aside for the moment, why catch and re-raise the exception?

[rashadalsharpini]

I simplified the code structure

• lost the if not which as you mentioned you can easily forget it's existence
• Moved cache check to the beginning
• Removed redundant exception
  i think this does it
  @orlitzky @fchapoton so what do you think?

[orlitzky]

It looks like an extra space snuck in here between self. and poly_x.

[orlitzky]

here too

[orlitzky]

Aside from three little typos it LGTM. Can you please add a doctest for the issue in #41400?

[rashadalsharpini]

i fixed typo i just got used smashing the space button :)
and i added the doctest for the issue @orlitzky

[dimpase]

@roed314 @saraedum - please trigger CI

[github-actions]

Documentation preview for this PR (built with commit aa049b7; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[rashadalsharpini]

any pointers on why these test failed, i didn't understand why or what happened
i tested the geometry module in whole and it worked fine @dimpase @fchapoton

[dimpase]

the test failure is irrelevant here, it's in

src/bin/sage -t --warn-long 5.0 --random-seed=23791240913723309037194613395941406430 src/sage/calculus/integration.pyx  # 1 doctest failed

[dimpase]

it would be good to get to the bottom of the matter,
by looking at constants defined in
http://hep.itp.tuwien.ac.at/~kreuzer/CY/palp/Global.h
which govern the max number of facets, etc.
@orlitzky

[orlitzky]

There's a git repo now at https://gitlab.com/stringstuwien/PALP

The error message is

Transpose_PM failed because #eq=76 > VERT_Nmax

which makes sense given that VERT_Nmax should be 64 for the 11d executable.

Fixing the problem in general is not going to be easy. With the upper limits hard-coded, you'll always be able to find a polytope that exceeds them. I think if a clever data structure was used, all of these limits could be dynamic, but that would (a) be a lot of work and (b) invalidate the documentation, which the author of PALP does not want to do.

We might be able to get the limit raised to 96 upstream, though. It can be done at build-time but if we expect the test suite to pass, we don't want certain polytopes to work only on certain distros.

[rashadalsharpini]

i closed it by mistake @dimpase it's ok right ?
since the 2 failing test are completely unrelated with my edits
do you merge it?
or do you normally schedule merge or what ? i just don't know

[orlitzky]

Closing is no big deal. Eventually someone will mark this "positive review" and then when it's time to release the next beta, our release manager will merge it.

I think your doctest misses the issue though. You have to run Q.points() twice to observe the bug, so in your doctest you should run it twice and see a RuntimeError both times.

[rashadalsharpini]

thanks, on my manual testing i used to running it more than once
i just forgot while writing it

[orlitzky]

The diff is OK, github is mangling it because the entire function was de-indented by one level.

One last comment (sorry). The description of the doctest does not really explain the problem. It says that the example used to fail, but then the doctest shows it failing again. If you can please make it say something like "This method should not cache an incorrect answer if an exception is raised."

[rashadalsharpini]

no problem, what do you think of this line? i think this explain it better

[orlitzky]

That's better, thank you.

[orlitzky]

@roed314 @saraedum can we have another CI run please for good measure?