The CremonaDatabase class does not work correctly.

[culler]

Steps To Reproduce

• build sage without the spkg database_cremona_ellcurve installed
• start sage
• Instantiate an object of class CremonaDatabase using the option mini=False

Expected Behavior

Sage should report that the database_cremona_ellcurve spkg is not installed and advise using mini=True.

Sage should not silently replace the large database by the mini database when the large database was specifically requested.

Actual Behavior

% venv/bin/sage
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 10.5.rc2, Release Date: 2024-11-30                │
│ Using Python 3.12.5. Type "help()" for help.                       │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: C = CremonaDatabase(mini=False)
sage: isinstance(C, sage.databases.cremona.MiniCremonaDatabase)
True

Additional Information

No response

Environment

• macOS (and surely all other OS's):
• 10.5:

Checklist

• I have searched the existing issues for a bug report that matches the one I want to file, without success.
• I have read the documentation and troubleshoot guide

[culler]

Since LargeCremonaDatabase is a subclass of MiniCremonaDatabase, I should have added one more test:

sage: C = CremonaDatabase(mini=False)
sage: isinstance(C, sage.databases.cremona.MiniCremonaDatabase)
True
sage: isinstance(C, sage.databases.cremona.LargeCremonaDatabase)
False

[user202729]

The fix is pretty simple…

     if set_global is not None:
         from sage.misc.superseded import deprecation
         deprecation(25825, "the set_global argument for CremonaDatabase is deprecated and ignored")
+    input_mini = mini
     if name is None:
         if DatabaseCremona().is_present():
             name = 'cremona'
@@ -1697,6 +1731,8 @@ def CremonaDatabase(name=None, mini=None, set_global=None):
         mini = True
     if mini is None:
         raise ValueError('mini must be set as either True or False')
+    if (name == 'cremona' and input_mini is True) or (name == 'cremona mini' and input_mini is False):
+        raise ValueError(f'wrong value of mini, database {name!r} must have mini={mini}')
 
     if mini:
         return MiniCremonaDatabase(name)

but the doctest is difficult to implement.

+
+    Verify that :issue:`39072` has been resolved::
+
+        sage: # optional - NOT database_cremona_ellcurve
+        sage: c = CremonaDatabase(mini=True)  # without database_cremona_ellcurve, this will select name = 'cremona mini'
+        sage: isinstance(c, sage.databases.cremona.MiniCremonaDatabase)
+        True
+        sage: c = CremonaDatabase(mini=False)  # same as above but invalid value of ``mini`` raises error
+        Traceback (most recent call last):
+        ...
+        ValueError: wrong value of mini, database 'cremona mini' must have mini=True
+        sage: c = CremonaDatabase(name='cremona mini', mini=False)
+        Traceback (most recent call last):
+        ...
+        ValueError: wrong value of mini, database 'cremona mini' must have mini=True
+
+    ::
+
+        sage: # optional - database_cremona_ellcurve
+        sage: c = CremonaDatabase(mini=False)
+        sage: isinstance(c, sage.databases.cremona.LargeCremonaDatabase)
+        True
+        sage: c = CremonaDatabase(mini=True)  # with database_cremona_ellcurve, this will select name = 'cremona'
+        Traceback (most recent call last):
+        ...
+        ValueError: wrong value of mini, database 'cremona' must have mini=False
+        sage: c = CremonaDatabase(name='cremona', mini=True)
+        Traceback (most recent call last):
+        ...
+        ValueError: wrong value of mini, database 'cremona' must have mini=False
+        sage: c = CremonaDatabase(name='cremona', mini=False)
+        sage: isinstance(c, sage.databases.cremona.LargeCremonaDatabase)
+        True

the current doctest framework does not support # optional - NOT database_cremona_ellcurve yet (read: this test is only expected to pass if optional feature database_cremona_ellcurve is not available)

Opinion: what should the doctest format be? Is # needs database_cremona_ellcurve uninstalled good? Problem is # needs takes multiple space-separated arguments, so we may get confusing situation like #needs a b uninstalled (read: a must be installed, b must be uninstalled) etc.?

[culler]

That's great that there is a simple fix.

I do wonder, however, why it is considered a good idea to make the user jump through these hoops, i.e. to be forced to provide redundant information and to be punished for doing so in an inconsistent way.

A more straightforward design would drop the name and mini arguments altogether and have one keyword option "size" which defaults to "auto" but can also be "large" or "mini". Then the CremonaDatabase function would return an instance of MiniCremonaDatabase if the "size" option is "mini", and either return an instance of LargeCremonaDatabase or raise a FeatureNotPresentError if the option is "large" . The default would be to provide the best available. A doctest should be able to test if the feature exists in order to know which result is expected.

This would differ from the current implementation and would therefore create a backwards incompatibility. But the current implementation is broken. A partial remedy would be to generate a deprecation warning if the keywords "name", "mini" or "setglobal" are used and to return a MiniCremonaDatabase in those cases unless mini is False and the large database is installed.

[kwankyu]

#39179 fixes the issue as expected, keeping the original semantics of arguments.