Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed warning while calling OEISSequence.is_dead() function #36811

Merged
merged 8 commits into from
Dec 26, 2023

Conversation

amanmoon
Copy link
Contributor

@amanmoon amanmoon commented Dec 4, 2023

Fixes #36795
fixed warning in OEISSequence.is_dead() function in OEISSequence

added a parameter warn=True in _field function
and changed self.is_dead(warn_only=True) to self.is_dead(warn_only=warn) inside the _field function,
changed the self._field('K') to self._field('K', warn=False)
so the warning is not triggered

@mantepse
Copy link
Collaborator

With this PR, in a fresh session,

sage: oeis(17).keywords()
('dead',)
sage: oeis(17)
A000017: Erroneous version of A032522.

so we don't get any warning at all if we first ask for the keywords. I think that's OK, so if noone objects, this can go in.

One thing I just noticed - the documentation of is_dead reads:

        A warn_only test is triggered as soon as some information on the
        sequence is queried::

I think it would be an improvement to write

        A warning is triggered if any field of a dead sequence is accessed,
        unless :meth:`is_dead` is called before::

@mantepse
Copy link
Collaborator

Unfortunately, testing with

sage -t --optional=sage,internet src/sage/databases/oeis.py

reveals a new failure, which is real and should be fixed:

File "src/sage/databases/oeis.py", line 2016, in sage.databases.oeis.OEISSequence.test_compile_sage_code
Failed example:
    s.test_compile_sage_code()            # optional -- internet
Expected:
    doctest:warning
    ...
    RuntimeWarning: This sequence is dead: ...
    True
Got:
    True

I think one can fix this by adding an additional call self.is_dead(warn_only=True).

There are other serious failures, as noted by @seblabbe, https://groups.google.com/g/sage-release/c/ow2oou9Q2dk/m/WCRtC3AsAQAJ, but these should be fixed in a separate ticket.

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

@mantepse - please try pressing "Approve and run" button

@mantepse

This comment was marked as off-topic.

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

I see the following (the button is 2/3rd down from the top)

image

@mantepse

This comment was marked as off-topic.

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

@mantepse - I don't know what's going on here. I think every member of https://github.com/orgs/sagemath/teams/triage
has access to this button.
And I see on my admin pages that

sagemath/sage
@mantepse mantepse has triage access over this repository 

What browser/OS are you on? Do you use 2FA?

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

anyhow, someone has just allowed a CI run - @mantepse - was it you?

@mantepse

This comment was marked as off-topic.

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

@mantepse - could you please log out and log in again?

Do you have 2FA set up for GitHub?

@mantepse

This comment was marked as off-topic.

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

Can I have a look at the bottom part of #36830 - between the last comment and the bottom ? (Either here or by email).

Firefox makes it easy to take screenshots of its pages (right click on the whitespace in the page, etc).

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

@mantepse - I'll remove you from triage, and send you an invite. Maybe this will clean this up...

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

@mantepse - I'll remove you from triage, and send you an invite. Maybe this will clean this up...

OK, I did this, and you got re-added to Triage. Has it helped?

@dimpase
Copy link
Member

dimpase commented Dec 13, 2023

@mantepse - another thing, to exclude browser issues. Do you have another machine you can try this on?

Do you use gh https://cli.github.com/ ? If so, you'd have ability to trigger runs etc.

@mantepse

This comment was marked as off-topic.

@amanmoon
Copy link
Contributor Author

I added a warn=True in keywords method, this will ensure only oeis(17).is_dead() will return True without a warning.
This also solves the problem of oeis(17).keywords() and s.test_compile_sage_code() not returning a warning.

@mantepse

This comment was marked as off-topic.

@mantepse
Copy link
Collaborator

I added a warn=True in keywords method, this will ensure only oeis(17).is_dead() will return True without a warning. This also solves the problem of oeis(17).keywords() and s.test_compile_sage_code() not returning a warning.

I'm sorry, but something went wrong here: I now get the warning no matter what I do:

sage: s = oeis(17)
sage: s.is_dead(warn_only=False)
/home/martin/sage/src/sage/databases/oeis.py:1153: RuntimeWarning: This sequence is dead: "A000017: Erroneous version of A032522."
  warn('This sequence is dead: "{}: {}"'.format(self.id(), self.name()), RuntimeWarning)
True

I think that it almost worked before the last commit, wouldn't it be sufficient to do the following? Or do you see other problems with this approach?

diff --git a/src/sage/databases/oeis.py b/src/sage/databases/oeis.py
index 867c66a757..2b8998f419 100644
--- a/src/sage/databases/oeis.py
+++ b/src/sage/databases/oeis.py
@@ -2020,6 +2020,7 @@ class OEISSequence(SageObject, UniqueRepresentation):
             True
         """
         if self.is_dead():
+            self.is_dead(warn_only=True)
             return True
         filt = self.programs(language='sage')
         if filt:

@amanmoon
Copy link
Contributor Author

amanmoon commented Dec 14, 2023

I thought the above changes will also solve the problem of oeis(17).keywords() not returning a warning.

@@ -2019,7 +2019,7 @@ def test_compile_sage_code(self):
RuntimeWarning: This sequence is dead: ...
True
"""
if self.is_dead():
if self.is_dead(warn_only=True):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not look right: self.is_dead(warn_only=True) will always return None. The bug does not surface, because A000017 does not have sage code.

@amanmoon
Copy link
Contributor Author

Whenever is_dead() method is called it loops from
is_dead() [where warn_only is initially false] => keywords() => _field() => is_dead() [warn_only becomes True and return warning if 'dead' is returned in keywords() method]

I added a third argument in is_dead() so whenever it loops from
is_dead() [where warn_only is initially false] => keywords() => _field() => is_dead() [warn_only remains False cause of the third argument]

@mantepse
Copy link
Collaborator

Did you see #36811 (comment) ?

@amanmoon
Copy link
Contributor Author

Yes, I did but it does not solve the problem of oeis(17).keywords() not returning a warning.
If it is fine for oeis(17).keywords() to not return a warning then i can revert the changes and add self.is_dead(warn_only=True).

@mantepse
Copy link
Collaborator

Yes, I did but it does not solve the problem of oeis(17).keywords() not returning a warning. If it is fine for oeis(17).keywords() to not return a warning then i can revert the changes and add self.is_dead(warn_only=True).

I think it is ok if - in a fresh session,

sage: oeis(17).keywords()

warns, but - in a fresh session

sage: oeis(17).is_dead()
True
sage: oeis(17).keywords()

does not warn.

Copy link

Documentation preview for this PR (built with commit 8f9af0b; changes) is ready! 🎉

Copy link
Collaborator

@mantepse mantepse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you so much!

If you have energy, it would be great if you could open an issue for the other issues, mentioned in #36811 (comment)!

If I did not overlook anything, the main issue is a weird change to the %I field, which seems to contain a date now.

vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 24, 2023
…function

    
Fixes sagemath#36795
fixed warning in OEISSequence.is_dead() function in OEISSequence

added a parameter warn=True in _field function
and changed self.is_dead(warn_only=True) to self.is_dead(warn_only=warn)
inside the _field function,
changed the self._field('K') to self._field('K', warn=False)
so the warning is not triggered
    
URL: sagemath#36811
Reported by: Aman Moon
Reviewer(s): Martin Rubey
@vbraun vbraun merged commit 003c1d9 into sagemath:develop Dec 26, 2023
19 of 20 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 26, 2023
@amanmoon amanmoon deleted the oeis_changes branch December 26, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot detect whether a sequence from the oeis is dead without warning
5 participants