#20382: Replace is_package_installed with Features
-------------------------------------+-------------------------------------
Reporter: saraedum | Owner:
Type: enhancement | Status: new
Priority: major | Milestone: sage-7.2
Component: build | Resolution:
Keywords: | Merged in:
Authors: Julian Rüth | Reviewers: Nicolas M. Thiéry,
Report Upstream: N/A | ...
Branch: | Work issues:
u/saraedum/replace_is_package_installed_with_features| Commit:
Dependencies: | 2a594ee16f72f7bebd06f82d0fdadfe8a526f178
| Stopgaps:
-------------------------------------+-------------------------------------
Comment (by saraedum):
Replying to [comment:16 vdelecroix]:
> Moreover #20182 implements some ''database of available features''
which
> is not discussed here. The latter has the advantage of minimizing the
> requests time after the first call. With the architecture proposed
here,
> each check involves a `Feature` class creation and some associated
test...
The docstring tells you that you might want to cache the result if you
think it
is a performance problem. Most of the time you are going to call out to a
binary anyway so I do not see the point of caching it as a default.
"Premature
optimization is the root of all evil…"
We could centralize the checks. I do not really have an opinion on that.
> - Do we really care about the class `FeatureTestResult`? If the result
is
> `True` then we mostly don't care about explanation and if it is `False`
we
> likely want an error (with an explicit error message).
There are places where the default algorithm is chosen depending on the
presence of some feature.
> Similarly, I would
> get rid of `FeatureNotPresentError`. Instead, I would add an `error`
and a
> `message` arguments in the `require` method (with of course some
> reasonable default which would mimic the current behavior using the
> standard `RuntimeError`).
So you have to catch whatever RuntimeError instead of being able to catch
something specific? I do not see the advantage of that. Also, if I
understand
your proposal, each caller has to cook up their own error message. I think
it
is nice to have a central place where these error messages and resolutions
live.
Finally, with your approach you do not get the granularity between `abc is
not
on the path` vs `abc is there but does not work`. That's something that
some
people requested.
> - Are you sure `CSDP` is better inside the file `lovasz_theta.py`? Do
we
> want to spread the `Features` all around the modules or should they be
> centralized?
See above.
> - Finally class names might be confusing. `GapPackage` is not a handle
on
> a gap package nor something describing what is inside a given package
> (what I would guess from the name).
GapPackage lives in sage.misc.feature. So it should be clear that this is
not
the actual package. We can suffix everything with Feature if that is
preferred.
I prefer short class names if possible.
> Similarly, with having `CSDP` has a
> class when `CSDP` is the name of the program. It would be fine if all
of
> them belong to a fixed module.
See above. (Sorry if I misunderstood you. It seems you are raising the
centralization vs localization issue several times in your comment.)
--
Ticket URL: <http://trac.sagemath.org/ticket/20382#comment:20>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at https://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
