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

Dev guide #353

Merged
merged 67 commits into from
Jan 28, 2025
Merged

Dev guide #353

merged 67 commits into from
Jan 28, 2025

Conversation

dpanici
Copy link
Collaborator

@dpanici dpanici commented Dec 14, 2022

Resolves #228
Resolves #775

  • Adds dev guide for transforms
  • Adds dev guide for backend and some JAX tricks
  • Adds dev guide on accessing derivatives and related things (maybe we might want to make this more visible for users)
  • Moves dev_guide files into docs/dev_guide folder

to do (or not to do, do we still want these?):

  • vmec.ipynb
  • perturbations.ipynb
  • fast fourier stuff on transform.ipynb

I am not sure how much detail I should give, but here is the first version, I can elaborate depending on your feedback.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Jan 11, 2023

Codecov Report

Merging #353 (60935d9) into master (a2fe711) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #353      +/-   ##
==========================================
- Coverage   94.26%   94.26%   -0.01%     
==========================================
  Files          78       78              
  Lines       18176    18176              
==========================================
- Hits        17134    17133       -1     
- Misses       1042     1043       +1     
Files Changed Coverage Δ
desc/compute/_core.py 100.00% <ø> (ø)
desc/compute/_equil.py 100.00% <ø> (ø)
desc/compute/_field.py 100.00% <ø> (ø)
desc/compute/_geometry.py 81.57% <ø> (ø)
desc/compute/_metric.py 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

@unalmis unalmis self-assigned this Jan 12, 2023
@unalmis unalmis added documentation Add documentation or better warnings etc. theory Requires theory work before coding labels Apr 12, 2023
@YigitElma YigitElma marked this pull request as ready for review January 23, 2025 23:40
@YigitElma YigitElma added the easy Short and simple to code or review label Jan 24, 2025
docs/index.rst Outdated Show resolved Hide resolved
Copy link
Member

@f0uriest f0uriest left a comment

Choose a reason for hiding this comment

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

Few minor fixes

docs/dev_guide/notebooks/backend.ipynb Show resolved Hide resolved
@@ -0,0 +1,227 @@
{
Copy link
Collaborator

@unalmis unalmis Jan 27, 2025

Choose a reason for hiding this comment

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

If we are going to mention this, then we should say "For example..., or we need the operation to be performed in a static manner rather than traced dynamically at run time." That is the main reason I would use numpy instead of JAX when developing.


Reply via ReviewNB

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point, but can you use it inside jit function? I don't think so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We do use numpy inside jitted and differentiable functions

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you google the "static jax" the relevant doc comes up

Use numpy for operations that you want to be static; use jax.numpy for operations that you want to be traced.

Copy link
Collaborator

@unalmis unalmis Jan 27, 2025

Choose a reason for hiding this comment

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

for example search for np.argsort in integrals/_interp_utils. It would be bad to use jax there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But I think that has some other drawbacks like multiple compilations etc. For every different value of the static input, the function has to compiled again. Anyway, I am making some changes.

Copy link
Collaborator

@unalmis unalmis Jan 27, 2025

Choose a reason for hiding this comment

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

the input for the numpy array there is not a jax array so it doesn't affect compilation

docs/dev_guide/notebooks/backend.ipynb Show resolved Hide resolved
docs/dev_guide/notebooks/transform.ipynb Show resolved Hide resolved
unalmis
unalmis previously approved these changes Jan 27, 2025
Copy link
Collaborator

@unalmis unalmis left a comment

Choose a reason for hiding this comment

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

left some comments, would be nice to make those quick updates as you address Rory's

@unalmis
Copy link
Collaborator

unalmis commented Jan 27, 2025

and thanks

@YigitElma YigitElma requested a review from f0uriest January 27, 2025 19:44
@YigitElma YigitElma merged commit 9bff6fd into master Jan 28, 2025
23 checks passed
@YigitElma YigitElma deleted the dev_guide branch January 28, 2025 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Add documentation or better warnings etc. easy Short and simple to code or review theory Requires theory work before coding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tutorial: Advanced AD / multi-gpu parallelism Useful Tutorial/Examples
4 participants