search

pascalmolin / hcperiods

Period matrice and Abel-Jacobi map of hyperelliptic and superperelliptic curves
MIT License
6 stars 1 forks source link

New changes #7

Closed christianneurohr closed 7 years ago

christianneurohr commented 7 years ago

See git commit comments for details.

pascalmolin commented 7 years ago

Several general things first:

pascalmolin commented 7 years ago

Now concerning nth_root_turn : you can simplify. There is no need to multiply by -1 each time, just accumulate in an int and change sign at the end. And the signs can just be ints, not arb types. Also you can remove blank lines at the end.

christianneurohr commented 7 years ago

Thanks for the comments and updates. It were my first steps with arb, I think it will be better soon!

2016-12-16 21:22 GMT+01:00 pascalmolin notifications@github.com:

@pascalmolin commented on this pull request.

In arb/intersections_tree.c https://github.com/pascalmolin/hcperiods/pull/7#pullrequestreview-13404657 :

     for (l = k + 1; l < d - 1; l++)

{ edge_t el = tree->e[l];

  • if (el.a != ek.a && el.a != ek.b)
  • if (ek.a != el.a && ek.b != el.a) / no intersection /

nice catch

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pascalmolin/hcperiods/pull/7#pullrequestreview-13404657, or mute the thread https://github.com/notifications/unsubscribe-auth/AFqZFpdBy-ei7gpEq41YpGDrIvX8rK_eks5rIvL9gaJpZM4LPajh .

pascalmolin commented 7 years ago

No problem. I will push the intersection tree file in a few minutes, I included your changes and I will add a test file. As you may have guessed from the code, I rewrote your theorem 5.1 in a more compact way.

Then I think it will be easier to close the pull request so that you start a new one with only a 2nd version of the nth_root.

An easy way to get started would also be to contribute tests, you see how they are written. Some could check the output is the same as the one you got from magma. Do not hesitate to use branches to avoid messing up everything when a commit is finally not taken.

Pascal

2016-12-16 21:55 GMT+01:00 christianneurohr notifications@github.com:

Thanks for the comments and updates. It were my first steps with arb, I think it will be better soon!

2016-12-16 21:22 GMT+01:00 pascalmolin notifications@github.com:

@pascalmolin commented on this pull request.

In arb/intersections_tree.c https://github.com/pascalmolin/hcperiods/pull/7# pullrequestreview-13404657 :

for (l = k + 1; l < d - 1; l++) { edge_t el = tree->e[l];

  • if (el.a != ek.a && el.a != ek.b)
  • if (ek.a != el.a && ek.b != el.a) / no intersection /

nice catch

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pascalmolin/hcperiods/pull/7# pullrequestreview-13404657, or mute the thread https://github.com/notifications/unsubscribe-auth/AFqZFpdBy- ei7gpEq41YpGDrIvX8rK_eks5rIvL9gaJpZM4LPajh .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/pascalmolin/hcperiods/pull/7#issuecomment-267693710, or mute the thread https://github.com/notifications/unsubscribe-auth/ADL0jfgtVEnS8897qZIqFIXY7544aa7Qks5rIvqwgaJpZM4LPajh .

christianneurohr commented 7 years ago

Hello Pascal,

i pushed the reworked nth_root. I like your simplification of Thm 5.1., you think it should be like that in the paper as well? Also, your Makefile isn't working for me (cannot 'make' as it doesnt find arb.h) and I don't know how to run the test files, any idea how to fix that?

Christian

2016-12-16 22:27 GMT+01:00 pascalmolin notifications@github.com:

No problem. I will push the intersection tree file in a few minutes, I included your changes and I will add a test file. As you may have guessed from the code, I rewrote your theorem 5.1 in a more compact way.

Then I think it will be easier to close the pull request so that you start a new one with only a 2nd version of the nth_root.

An easy way to get started would also be to contribute tests, you see how they are written. Some could check the output is the same as the one you got from magma. Do not hesitate to use branches to avoid messing up everything when a commit is finally not taken.

Pascal

2016-12-16 21:55 GMT+01:00 christianneurohr notifications@github.com:

Thanks for the comments and updates. It were my first steps with arb, I think it will be better soon!

2016-12-16 21:22 GMT+01:00 pascalmolin notifications@github.com:

@pascalmolin commented on this pull request.

In arb/intersections_tree.c https://github.com/pascalmolin/hcperiods/pull/7# pullrequestreview-13404657 :

for (l = k + 1; l < d - 1; l++) { edge_t el = tree->e[l];

  • if (el.a != ek.a && el.a != ek.b)
  • if (ek.a != el.a && ek.b != el.a) / no intersection /

nice catch

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pascalmolin/hcperiods/pull/7# pullrequestreview-13404657, or mute the thread https://github.com/notifications/unsubscribe-auth/AFqZFpdBy- ei7gpEq41YpGDrIvX8rK_eks5rIvL9gaJpZM4LPajh .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/pascalmolin/hcperiods/pull/7#issuecomment-267693710 , or mute the thread https://github.com/notifications/unsubscribe-auth/ ADL0jfgtVEnS8897qZIqFIXY7544aa7Qks5rIvqwgaJpZM4LPajh .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pascalmolin/hcperiods/pull/7#issuecomment-267699978, or mute the thread https://github.com/notifications/unsubscribe-auth/AFqZFhf-CdBJmk7PPadBeLxal0FA34Bfks5rIwJAgaJpZM4LPajh .