#28422 closed defect (fixed)

Change keyword argument 'verbose' to 'check' in CoordChange.set_inverse() and set the default to True

Reported by: egourgoulhon Owned by:
Priority: major Milestone: sage-8.9
Component: geometry Keywords: manifolds, chart, coordinate_change
Cc: SimonKing, tscrim Merged in:
Authors: Eric Gourgoulhon Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: a9c5ad8 (Commits) Commit: a9c5ad86d706ea479d3c0525a5196d413a22c541
Dependencies: Stopgaps:

Description (last modified by egourgoulhon)

As pointed out in this sage-devel post, the keyword verbose=True used to trigger the check of the inverse of a transition map in CoordChange.set_inverse() would be better named check=True. This is performed in this ticket. Moreover, check=True is now the default (previously, it was verbose=False) and the printed output of the check has been improved.

Change History (15)

comment:1 Changed 13 months ago by egourgoulhon

  • Branch set to public/manifolds/check_set_inverse-trac28422
  • Cc SimonKing tscrim added
  • Commit set to 26a47e058dfb941c378c3e0e17f1684611b57691
  • Status changed from new to needs_review

New commits:

26a47e0Change keyword argument 'verbose' to 'check' in CoordChange.set_inverse() and set the default to True

comment:2 Changed 13 months ago by egourgoulhon

  • Summary changed from Change keyword argument 'verbose' to 'check' in CoordChange.set_inverse(() to Change keyword argument 'verbose' to 'check' in CoordChange.set_inverse()

comment:3 Changed 13 months ago by egourgoulhon

  • Description modified (diff)
  • Summary changed from Change keyword argument 'verbose' to 'check' in CoordChange.set_inverse() to Change keyword argument 'verbose' to 'check' in CoordChange.set_inverse() and set the default to True

comment:4 follow-up: Changed 13 months ago by tscrim

I think you should also have a verbose option whose default is False so it doesn't print all the information with the check (as we should assume the check pass).

comment:5 in reply to: ↑ 4 ; follow-up: Changed 13 months ago by egourgoulhon

Replying to tscrim:

I think you should also have a verbose option whose default is False so it doesn't print all the information with the check (as we should assume the check pass).

If we restore a verbose option, then there is no point in having check=True since the printed output is the only outcome of the check. In particular, no exception is raised if the check is failed. The appreciation of whether the check is passed or not is thus left to the user. This is so because the check can fail when Sage is not capable to simplify enough the output, as with the atan2 function considered in the EXAMPLES section of the documentation.

comment:6 in reply to: ↑ 5 Changed 13 months ago by egourgoulhon

Replying to egourgoulhon:

If we restore a verbose option, then there is no point in having check=True since the printed output is the only outcome of the check. In particular, no exception is raised if the check is failed. The appreciation of whether the check is passed or not is thus left to the user. This is so because the check can fail when Sage is not capable to simplify enough the output, as with the atan2 function considered in the EXAMPLES section of the documentation.

Taking into account the above, a possible use of a verbose option would the following: if check=True, then

  • for verbose=True, print the check details (as they are now)
  • for verbose=False, print nothing if the check is fully passed (i.e. if the variable any_failure defined in the code remains to False) and print the check details (as they are now) when some failure has occurred

Would you agree with such a behavior?

Last edited 13 months ago by egourgoulhon (previous) (diff)

comment:7 follow-up: Changed 13 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Ah, I think I misread the code the first time through. I thought it always printed stuff out, but it only does it on failures. So you can disregard my comment. This is good.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 13 months ago by egourgoulhon

Replying to tscrim:

Ah, I think I misread the code the first time through. I thought it always printed stuff out, but it only does it on failures.

Well actually, in the current version, with check=True, it always print something out, even if the check is passed (the only thing that is not printed in that case is the sentence "NB: a failed report can reflect a mere lack of simplification"). Personally, I don't feel uneasy with a function that is verbose when some "interactivity" is implicitly asked in the form "check=True".

comment:9 in reply to: ↑ 8 ; follow-up: Changed 13 months ago by tscrim

  • Status changed from positive_review to needs_work

Replying to egourgoulhon:

Replying to tscrim:

Ah, I think I misread the code the first time through. I thought it always printed stuff out, but it only does it on failures.

Well actually, in the current version, with check=True, it always print something out, even if the check is passed (the only thing that is not printed in that case is the sentence "NB: a failed report can reflect a mere lack of simplification"). Personally, I don't feel uneasy with a function that is verbose when some "interactivity" is implicitly asked in the form "check=True".

Okay, now I am reading the code more carefully. I don't like it being overly verbose when it passes successfully. I am happy when it prints something on failure, but I don't like the noise when it passes successfully. I feel it just gets in the way. So if you feel there is a benefit to having the data every time, then you should add a verbose as you proposed in comment:6.

Addendum - This objection is only when it is the default behavior. If this was something a user had to ask for, then it wouldn't be a problem.

Last edited 13 months ago by tscrim (previous) (diff)

comment:10 Changed 13 months ago by git

  • Commit changed from 26a47e058dfb941c378c3e0e17f1684611b57691 to a9c5ad86d706ea479d3c0525a5196d413a22c541

Branch pushed to git repo; I updated commit sha1. New commits:

a9c5ad8Add keyword verbose in CoordChange.set_inverse()

comment:11 in reply to: ↑ 9 Changed 13 months ago by egourgoulhon

Replying to tscrim:

Okay, now I am reading the code more carefully. I don't like it being overly verbose when it passes successfully. I am happy when it prints something on failure, but I don't like the noise when it passes successfully. I feel it just gets in the way. So if you feel there is a benefit to having the data every time, then you should add a verbose as you proposed in comment:6.

OK, this is done in the above commit.

Addendum - This objection is only when it is the default behavior. If this was something a user had to ask for, then it wouldn't be a problem.

OK I see your point.

comment:12 Changed 13 months ago by egourgoulhon

  • Status changed from needs_work to needs_review

comment:13 Changed 13 months ago by tscrim

  • Status changed from needs_review to positive_review

Thank you. LGTM (it is a bit strange that in Python invalid keywords are TypeErrors, but that is the convention).

comment:14 Changed 13 months ago by egourgoulhon

Thanks for the review!

comment:15 Changed 13 months ago by vbraun

  • Branch changed from public/manifolds/check_set_inverse-trac28422 to a9c5ad86d706ea479d3c0525a5196d413a22c541
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.