Opened 5 months ago

Closed 4 months ago

#32014 closed enhancement (fixed)

Add Gauss-Legendre vector integration with specified number of nodes

Reported by: gh-DisneyHogg Owned by: gh-DisneyHogg
Priority: major Milestone: sage-9.4
Component: numerical Keywords:
Cc: nbruin, slelievre Merged in:
Authors: Linden Disney-Hogg Reviewers: Samuel Lelièvre, Nils Bruin
Report Upstream: N/A Work issues:
Branch: 11feeb2 (Commits, GitHub, GitLab) Commit: 11feeb2ee1a3841a3eaaca42f0acb2dbd860eeaa
Dependencies: Stopgaps:

Status badges

Description (last modified by slelievre)

From #30698.

We add a method integrate_vector_N.

Change History (16)

comment:1 Changed 5 months ago by gh-DisneyHogg

  • Authors set to Linden Disney-Hogg
  • Cc nbruin slelievre added
  • Component changed from PLEASE CHANGE to numerical
  • Owner changed from (none) to gh-DisneyHogg
  • Summary changed from functionality_of_integrate_vector to functionality of integrate_vector
  • Type changed from PLEASE CHANGE to enhancement

As per ticket #30698, move the updates to sage.numerical.gauss_lengdre.integrate_vector, namely allowing for the specification of the number of nodes desired a priori, to a new ticket.

Last edited 5 months ago by gh-DisneyHogg (previous) (diff)

comment:2 Changed 5 months ago by gh-DisneyHogg

  • Branch set to u/gh-DisneyHogg/functionality_of_integrate_vector

comment:3 follow-up: Changed 5 months ago by gh-DisneyHogg

  • Commit set to b1d61ba69958bbdba7448193609a1def428d669a

Because a version of this ticket was started as part of #30698, this ticket should eventually be merged with that ticket, to resolve the conflict.


New commits:

b1d61baAdded integrate_vector_N method

comment:4 in reply to: ↑ 3 Changed 5 months ago by nbruin

Replying to gh-DisneyHogg:

Because a version of this ticket was started as part of #30698, this ticket should eventually be merged with that ticket, to resolve the conflict.

One way of dealing with these kinds of merge conflicts is to set the "Dependencies" field to #30698, and rebase this branch on the branch there. Once the tickets are ready to be merged, this clearly indicates the merge order. It may mean that the branch here must be merged with further developments on the other branch. It's actually a practical reason *not* to split tickets you're working on, if they are mainly touching the same file.

You can also hope that git's standard merge tools recognize you're touching different lines in the same file and hence merge without reporting conflicts (logically, the changes on the tickets are orthogonal, so the dependence would really be for practical reasons)

comment:5 Changed 5 months ago by gh-DisneyHogg

  • Status changed from new to needs_review

comment:6 Changed 5 months ago by slelievre

  • Description modified (diff)
  • Reviewers set to Samuel Lelièvre
  • Summary changed from functionality of integrate_vector to Add Gauss-Legendre vector integration with specified number of nodes

Can you fix the typo "incomatible" for "incompatible" and maybe rephrase

- It is different from ``integrate_vector`` by fixing the number
- of nodes to use, rather than aiming for a certain error.
+ It differs from ``integrate_vector`` by using a specified number
+ of nodes rather than targeting a specified error bound on the result.

and remove the extra blank line at the end of the docstring?

comment:7 follow-up: Changed 5 months ago by mkoeppe

I think a better interface would be to just extend integrate_vector so that one can either use epsilon or the number of nodes as a keyword argument.

comment:8 in reply to: ↑ 7 Changed 5 months ago by slelievre

  • Description modified (diff)

Replying to mkoeppe:

I think a better interface would be to just extend integrate_vector so that one can either use epsilon or the number of nodes as a keyword argument.

There was a discussion at #30698.

comment:9 Changed 5 months ago by git

  • Commit changed from b1d61ba69958bbdba7448193609a1def428d669a to 7fc196b04f5708380b49195a4f2c0b1bcb07d396

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

7fc196bTypo fix and rephrasing.

comment:10 follow-up: Changed 5 months ago by nbruin

  • Status changed from needs_review to needs_work

Merge fails; branch needs rebasing.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 5 months ago by gh-DisneyHogg

Replying to nbruin:

Merge fails; branch needs rebasing.

Can you give more detail please? Were you merging #30698 into this ticket and it had a conflict?

comment:12 in reply to: ↑ 11 Changed 5 months ago by nbruin

Replying to gh-DisneyHogg:

Replying to nbruin:

Merge fails; branch needs rebasing.

Can you give more detail please? Were you merging #30698 into this ticket and it had a conflict?

If you look at the ticket description, you can see that the branch is displayed in red, and if you hover over it, you see "trac's automerging failed". If plain merging by git gives no problem, this indicates that trac tries a merge with particularly conservative and sage settings. However, I think you'll at least have to make sure that trac can do the merge, because otherwise reviewers have a harder time seeing what's happening on the ticket.

(I suspect the problem is that the current branch makes changes to an ancestor of "develop" that end up identical to what eventually happens in "develop", but are proposed separately. It would for instance mean that the "blame" of the relevant lines would be ambiguous. While in university administration and politics, this would be a valuable feature, it is slightly undesirable for the modification history of a software project)

comment:13 Changed 4 months ago by git

  • Commit changed from 7fc196b04f5708380b49195a4f2c0b1bcb07d396 to 11feeb2ee1a3841a3eaaca42f0acb2dbd860eeaa

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

1c7cb5bMerge branch 'develop' into t/32014/functionality_of_integrate_vector
11feeb2Merge branch 'develop' into t/32014/functionality_of_integrate_vector and fix conflict.

comment:14 Changed 4 months ago by gh-DisneyHogg

  • Status changed from needs_work to needs_review

Merge fail has been fixed.

comment:15 Changed 4 months ago by nbruin

  • Reviewers changed from Samuel Lelièvre to Samuel Lelièvre, Nils Bruin
  • Status changed from needs_review to positive_review

Documentation has been adjusted, routine itself is basically trivial (but necessary for upcoming tickets), so: positive review!

comment:16 Changed 4 months ago by vbraun

  • Branch changed from u/gh-DisneyHogg/functionality_of_integrate_vector to 11feeb2ee1a3841a3eaaca42f0acb2dbd860eeaa
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.