added check for invalid range in contour_plot and derivatives #35113

jnash10 · 2023-02-13T11:39:14Z

📚 Description

Add if condition in misc.py which contour_plot uses to check for invalid range entered by user(xmin > xmax or ymin>ymax)

📝 Checklist

[x ] I have made sure that the title is self-explanatory and the description concisely explains the PR.
[ x] I have linked an issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

codecov-commenter · 2023-02-16T11:24:10Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (05329f6) 88.59% compared to head (cd0c618) 88.59%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35113      +/-   ##
===========================================
- Coverage    88.59%   88.59%   -0.01%     
===========================================
  Files         2140     2140              
  Lines       396998   397004       +6     
===========================================
- Hits        351740   351739       -1     
- Misses       45258    45265       +7

Impacted Files	Coverage Δ
src/sage/plot/misc.py	`97.50% <100.00%> (+0.13%)`	⬆️
...e/combinat/cluster_algebra_quiver/mutation_type.py	`52.85% <0.00%> (-1.79%)`	⬇️
src/sage/groups/generic.py	`88.34% <0.00%> (-0.68%)`	⬇️
src/sage/schemes/elliptic_curves/hom.py	`83.33% <0.00%> (-0.62%)`	⬇️
src/sage/graphs/generators/random.py	`91.95% <0.00%> (-0.52%)`	⬇️
src/sage/quadratic_forms/binary_qf.py	`92.78% <0.00%> (-0.50%)`	⬇️
src/sage/graphs/tutte_polynomial.py	`93.57% <0.00%> (-0.46%)`	⬇️
src/sage/schemes/elliptic_curves/hom_velusqrt.py	`94.48% <0.00%> (-0.40%)`	⬇️
src/sage/modular/arithgroup/arithgroup_perm.py	`92.57% <0.00%> (-0.38%)`	⬇️
...sage/geometry/hyperbolic_space/hyperbolic_model.py	`88.95% <0.00%> (-0.31%)`	⬇️
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kwankyu · 2023-02-19T02:21:34Z

@tobiasdiez Would you approve the workflows if you have the permission?

kwankyu · 2023-02-19T02:25:21Z

src/sage/plot/misc.py

@@ -139,6 +139,13 @@ def setup_for_eval_on_grid(funcs,
    else:
        vars, free_vars = unify_arguments(funcs)

+    #check for invalid range entered by user


needs a space # check I would write # check for valid range

Look into the Build & Test check. There are lots of failed doctests. It seems that ranges[1] may not exist.

Solved this. Some functions take range input differently, so nested my if condition inside another which checks if:

len(ranges) > 1

Haven't pushed it yet due to the issue mentioned in the comment below.

src/sage/plot/misc.py

kwankyu · 2023-02-19T21:20:24Z

Look into the Build & Test check. There are lots of failed doctests. It seems that ranges[1] may not exist.

jnash10 · 2023-02-20T04:59:37Z

I think I found the error. One of the examples in the doctest is the following:

sage/src/sage/plot/misc.py

Lines 107 to 111 in 05329f6

    
                   ValueError: plot start point and end point must be different 
        
                   sage: sage.plot.misc.setup_for_eval_on_grid(x+y, [(x,1,-1),(y,-1,1)], return_vars=True) 
        
                   (<sage...>, [(1.0, -1.0, 2.0), (-1.0, 1.0, 2.0)], [x, y]) 
        
                   sage: sage.plot.misc.setup_for_eval_on_grid(x+y, [(y,1,-1),(x,-1,1)], return_vars=True) 
        
                   (<sage...>, [(1.0, -1.0, 2.0), (-1.0, 1.0, 2.0)], [y, x])

Which has the range in the wrong format as in example 1: xmin(=1) > xmax(=-1) and in example2 : ymin(=1) > ymax(=-1) which is against the definition of range :

sage/src/sage/plot/misc.py

Lines 34 to 36 in 05329f6

    
               - ``ranges`` -- a list of ranges.  A range can be a 2-tuple of 
        
                 numbers specifying the minimum and maximum, or a 3-tuple giving 
        
                 the variable explicitly.

Should I change the doctest example in this case and make it go from -1 to 1?

This test is failling the code edit as this is the very error it aims to catch.

kwankyu · 2023-02-20T05:14:41Z

It seems we could just swap the two values to fix the error, and use the doctests to test this conveniency.

jnash10 · 2023-02-20T05:19:36Z

It seems we could just swap the two values to fix the error, and use the doctests to test this conveniency.

Nice idea!

jnash10 · 2023-02-20T05:44:41Z

It seems we could just swap the two values to fix the error, and use the doctests to test this conveniency.

Upon implementing it, the plot comes out okay:

Yet the doctest fails as the intermediate function is also apparently returning a wrapper:

File "src/sage/plot/misc.py", line 110, in sage.plot.misc.?
Failed example:
    sage.plot.misc.setup_for_eval_on_grid(x+y, [(y,1,-1),(x,-1,1)], return_vars=True)
Expected:
    (<sage...>, [(1.0, -1.0, 2.0), (-1.0, 1.0, 2.0)], [y, x])
Got:
    (<sage.plot.misc.FastCallablePlotWrapper object at 0x7f2dc08daa10>,
     [(-1.0, 1.0, 2.0), (-1.0, 1.0, 2.0)],
     [y, x])

This was the implementation:

    # check for invalid range entered by user (xmin > xmax or ymin > ymax) and swap the values if True
    if len(ranges) > 1:
        for i in range(len(ranges)):
            if (ranges[i][-2] > ranges[i][-1]):
                ranges[i] = list(ranges[i])
                ranges[i][-1], ranges[i][-2] = ranges[i][-2], ranges[i][-1]
                ranges[i] = tuple(ranges[i])

Converting tuple to list to tuple was necessary for the swap( as tuples are immutable). But after the conversion since the data type is same, the response should also ideally be the same.

kwankyu · 2023-02-20T06:01:18Z

You can just fix the minor changes in the return values in the doctests.

That is,

    (<sage...>, [(-1.0, 1.0, 2.0), (-1.0, 1.0, 2.0)], [y, x])

kwankyu · 2023-02-20T06:04:37Z

Remove unnecessary parentheses in the condition of if statement.

jnash10 · 2023-02-20T06:20:28Z

Upon making this change, all tests are passed for misc.py

./sage -t ...../misc.py

giving me:

All tests passed!
----------------------------------------------------------------------
Total time for all tests: 4.0 seconds
    cpu time: 3.3 seconds
    cumulative wall time: 4.0 seconds

Is that enough or should I now run:

./sage -t -all

?

…ate docstest to account for the same

…lues if so

…lve_35032

kwankyu · 2023-02-20T06:46:29Z

Normally, you don't need to because workflows for checking the branch should be run by now. But, as you are first-time contributor, it seems that running workflows require admin's permission. @tobiasdiez may take care of you.

kwankyu · 2023-02-20T06:52:12Z

@mkoeppe @dimpase @roed314

3 workflows awaiting approval
This workflow requires approval from a maintainer. Learn more.

Could triage team have the permission to approve to run workflows for first-time (non-member) contributors?

roed314 · 2023-02-20T07:11:33Z

@kwankyu Unfortunately, we can't change the permissions on a fine-grained level without paying for Github's enterprise level. But I'll approve the run here.

kwankyu · 2023-02-20T07:31:49Z

Thanks. I wonder if we can run a bot approving workflows automatically for PRs with secure(?) branch and with certain label...

roed314 · 2023-02-20T09:12:08Z

I don't know how we would check that code is secure....

kwankyu · 2023-02-20T10:02:33Z

We may consider code is basically secure when there's no change in github workflows. Other than that, the reviewer should decide whether to trust the contributor and add a label to signify that. But I don't know the technical side whether github allows such a bot. Logically, it seems, the bot should not be a github workflow.

kwankyu

LGTM.

jnash10 · 2023-02-21T09:21:06Z

@kwankyu Thank you for bearing with all my doubts and walking me through this PR/issue.

Is there anything else I need to do for this particular PR/issue?
(squash commits etc.? and in that case, do the checks happen again?)

Thank you again for taking the time!

kwankyu · 2023-02-21T09:35:32Z

Is there anything else I need to do for this particular PR/issue? (squash commits etc.?

Once you get "positive review", you are all set.

Anyway, for this PR, if you are willing, please make the comment # check ... short (according to PEP8). You can also squash commits.

and in that case, do the checks happen again?

Yes (but no for this PR). But don't mind that.

jnash10 · 2023-02-23T04:25:49Z

src/sage/plot/misc.py

@@ -139,6 +139,14 @@ def setup_for_eval_on_grid(funcs,
    else:
        vars, free_vars = unify_arguments(funcs)

+    # check for invalid range entered by user (xmin > xmax or ymin > ymax) and swap the values if True


Updating comment to be shorter.

jnash10 · 2023-02-23T04:28:54Z

Is there anything else I need to do for this particular PR/issue? (squash commits etc.?

Once you get "positive review", you are all set.

Anyway, for this PR, if you are willing, please make the comment # check ... short (according to PEP8). You can also squash commits.

and in that case, do the checks happen again?

Yes (but no for this PR). But don't mind that.

Thank you.

Comment updated!

kwankyu

Looks good. Thank you.

jnash10 · 2023-02-23T11:08:16Z

Removed trailing whitespace in the comment.

kwankyu

Okay

github-actions · 2023-03-03T23:40:52Z

Documentation preview for this PR is ready! 🎉
Built with commit: cd0c618

added check for invalid range

604c916

jnash10 mentioned this pull request Feb 13, 2023

region_plot raises weird IndexError when y_min > y_max #35032

Closed

2 tasks

seblabbe mentioned this pull request Feb 13, 2023

Added case to catch invalid range by user in contour plot and derivatives #35077

Closed

kwankyu added t: bug p: minor / 4 s: needs work c: graphics labels Feb 19, 2023

kwankyu reviewed Feb 19, 2023

View reviewed changes

src/sage/plot/misc.py Show resolved Hide resolved

tscrim added t: enhancement and removed t: bug labels Feb 19, 2023

aesthetic changes to code

8ccb842

jnash10 force-pushed the solve_35032 branch from 9e9dfea to 8ccb842 Compare February 19, 2023 11:37

wrongisright and others added 5 commits February 20, 2023 11:52

Switch range values in case of invalid range (min > max) by user. Upd…

8435f06

…ate docstest to account for the same

Update misc.py

2962a9a

Added check for invalid range (xmin >xmax or ymin > ymax) and swap va…

72485ae

…lues if so

Merge branch 'solve_35032' of /~https://github.com/jnash10/sage into so…

3abf104

…lve_35032

Merge branch 'develop' into solve_35032

6396a7c

kwankyu added s: needs review and removed s: needs work labels Feb 20, 2023

kwankyu approved these changes Feb 20, 2023

View reviewed changes

kwankyu added s: positive review and removed s: needs review labels Feb 20, 2023

jnash10 commented Feb 23, 2023

View reviewed changes

Invalid range (xmin> xmax) check. Updated comment.

9671739

kwankyu approved these changes Feb 23, 2023

View reviewed changes

check for inavlid range. removed trailing whitespace

cd0c618

kwankyu approved these changes Feb 23, 2023

View reviewed changes

jnash10 mentioned this pull request Mar 19, 2023

Meta-ticket: Use Python optimization interfaces: CVXPY, SCIP, or-tools, PuLP, Pyomo, cylp... #26511

Open

vbraun merged commit f18723d into sagemath:develop Mar 26, 2023

kwankyu removed the s: positive review label Mar 26, 2023

mkoeppe added this to the sage-10.0 milestone Mar 26, 2023

jnash10 deleted the solve_35032 branch March 30, 2023 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added check for invalid range in contour_plot and derivatives #35113

added check for invalid range in contour_plot and derivatives #35113

jnash10 commented Feb 13, 2023

codecov-commenter commented Feb 16, 2023 •

edited

Loading

kwankyu commented Feb 19, 2023

kwankyu Feb 19, 2023

jnash10 Feb 20, 2023

kwankyu commented Feb 19, 2023 •

edited

Loading

jnash10 commented Feb 20, 2023 •

edited

Loading

kwankyu commented Feb 20, 2023

jnash10 commented Feb 20, 2023

jnash10 commented Feb 20, 2023

kwankyu commented Feb 20, 2023 •

edited

Loading

kwankyu commented Feb 20, 2023

jnash10 commented Feb 20, 2023

kwankyu commented Feb 20, 2023

kwankyu commented Feb 20, 2023 •

edited

Loading

roed314 commented Feb 20, 2023

kwankyu commented Feb 20, 2023 •

edited

Loading

roed314 commented Feb 20, 2023

kwankyu commented Feb 20, 2023

kwankyu left a comment

jnash10 commented Feb 21, 2023

kwankyu commented Feb 21, 2023 •

edited

Loading

jnash10 Feb 23, 2023

jnash10 commented Feb 23, 2023

kwankyu left a comment

jnash10 commented Feb 23, 2023

kwankyu left a comment

github-actions bot commented Mar 3, 2023

added check for invalid range in contour_plot and derivatives #35113

added check for invalid range in contour_plot and derivatives #35113

Conversation

jnash10 commented Feb 13, 2023

📚 Description

📝 Checklist

codecov-commenter commented Feb 16, 2023 • edited Loading

Codecov Report

kwankyu commented Feb 19, 2023

kwankyu Feb 19, 2023

Choose a reason for hiding this comment

jnash10 Feb 20, 2023

Choose a reason for hiding this comment

kwankyu commented Feb 19, 2023 • edited Loading

jnash10 commented Feb 20, 2023 • edited Loading

kwankyu commented Feb 20, 2023

jnash10 commented Feb 20, 2023

jnash10 commented Feb 20, 2023

kwankyu commented Feb 20, 2023 • edited Loading

kwankyu commented Feb 20, 2023

jnash10 commented Feb 20, 2023

kwankyu commented Feb 20, 2023

kwankyu commented Feb 20, 2023 • edited Loading

roed314 commented Feb 20, 2023

kwankyu commented Feb 20, 2023 • edited Loading

roed314 commented Feb 20, 2023

kwankyu commented Feb 20, 2023

kwankyu left a comment

Choose a reason for hiding this comment

jnash10 commented Feb 21, 2023

kwankyu commented Feb 21, 2023 • edited Loading

jnash10 Feb 23, 2023

Choose a reason for hiding this comment

jnash10 commented Feb 23, 2023

kwankyu left a comment

Choose a reason for hiding this comment

jnash10 commented Feb 23, 2023

kwankyu left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 3, 2023

codecov-commenter commented Feb 16, 2023 •

edited

Loading

kwankyu commented Feb 19, 2023 •

edited

Loading

jnash10 commented Feb 20, 2023 •

edited

Loading

kwankyu commented Feb 20, 2023 •

edited

Loading

kwankyu commented Feb 20, 2023 •

edited

Loading

kwankyu commented Feb 20, 2023 •

edited

Loading

kwankyu commented Feb 21, 2023 •

edited

Loading