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

Refactor surface and curve compute methods #583

Merged
merged 40 commits into from
Aug 8, 2023
Merged

Conversation

f0uriest
Copy link
Member

@f0uriest f0uriest commented Jul 15, 2023

  • Replaces the individual methods compute_coordinates, compute_normal etc of Curve and Surface objects with a generic compute method similar to the one used in the Equilibrium class.
  • This allows us to share a lot of the desc.compute infrastructure with new classes and reduces duplicated code.
  • Removes grid and transform attributes from Curve and Surface classes, these are now built on the fly in the compute method.
  • FourierXYZCurve now has seperate basis for X, Y, Z for consistency with other classes.

Resolves #559

f0uriest added 29 commits June 27, 2023 18:54
@f0uriest f0uriest marked this pull request as draft July 15, 2023 22:15
@f0uriest f0uriest changed the title Rc/compute geometry Refactor surface and curve compute methods Jul 31, 2023
@f0uriest f0uriest marked this pull request as ready for review July 31, 2023 17:46
@@ -22,6 +22,34 @@ def _V(params, transforms, profiles, data, **kwargs):
return data


@register_compute_fun(
name="V",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now there are two difference compute functions both with the name "V". I'm confused about when one is called over the other. Does this new one only get called for FourierRZToroidalSurface, and the original one gets called for all other classes? But this new one is actually more general, where as the original one will only work for an Equilibrium.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the new compute function is valid in all use cases, can we replace the old one instead of having two different functions that compute the same quantity?

If we need to keep both implementations, we should add a test to check them against each other.

Copy link
Member Author

Choose a reason for hiding this comment

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

The new one is more general and could replace the old one, I'll make that change.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually its a bit tricky. The new compute function is only correct if the grid has nodes along the LCFS, while the original one assumes quadrature nodes in the volume, which genreally are a bit short of the LCFS. in eq.compute we use a quadrature grid to compute everything with dim==0, but that will give an incorrect result with the new compute method (Surface.compute uses a linear grid so its correct using the new method)

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. It still seems messy to have redundant compute methods. Should we add a parameterization to the old method to specify it works for Equilibrium? We need something to document when to use one over the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

the default parameterization is Equilibrium unless otherwise specified. I don't think its much of an issue since users will never really interact with these functions directly.

desc/compute/utils.py Show resolved Hide resolved
@dpanici dpanici mentioned this pull request Aug 3, 2023
18 tasks
@f0uriest f0uriest requested a review from ddudt August 3, 2023 21:27
@dpanici
Copy link
Collaborator

dpanici commented Aug 4, 2023

I noticed a couple things

  • we don't have the same sort of logic setup in the curve and surface compute functions for ensuring that when we calculate things and a grid is passed in, that things stay broadcastable. Is that fine? I know for stuff like length it is a scalar and does not matter, and it was really more important for calculating 1D profiles that we wanted to then be able to multiple with 3D quantities, but is there nothing analagous for the Surface class let's say?
  • There isn't any logic to check that a passed-in grid has nonzero N resolution for the Curve compute function, or nonzero M,N resolution for the Surface compute function. That could be good to add

@f0uriest
Copy link
Member Author

f0uriest commented Aug 4, 2023

I noticed a couple things

* we don't have the same sort of logic setup in the curve and surface  compute functions for ensuring that when we calculate things and a grid is passed in, that things stay broadcastable. Is that fine? I know for stuff like length it is a scalar and does not matter, and it was really more important for calculating 1D profiles that we wanted to then be able to multiple with 3D quantities, but is there nothing analagous for the Surface class let's say?

* There isn't any logic to check that a passed-in grid has nonzero N resolution for the Curve compute function, or nonzero M,N resolution for the Surface compute function. That could be good to add

I dont think there are any broadcasting issues, since everything for curves and surfaces is either a scalar or a function over the whole curve/surface, so everything should work fine.
For the grids, I think its fine if it has N=0, ie, if you just want to compute something at a single point.

@dpanici
Copy link
Collaborator

dpanici commented Aug 4, 2023

I noticed a couple things

* we don't have the same sort of logic setup in the curve and surface  compute functions for ensuring that when we calculate things and a grid is passed in, that things stay broadcastable. Is that fine? I know for stuff like length it is a scalar and does not matter, and it was really more important for calculating 1D profiles that we wanted to then be able to multiple with 3D quantities, but is there nothing analagous for the Surface class let's say?

* There isn't any logic to check that a passed-in grid has nonzero N resolution for the Curve compute function, or nonzero M,N resolution for the Surface compute function. That could be good to add

I dont think there are any broadcasting issues, since everything for curves and surfaces is either a scalar or a function over the whole curve/surface, so everything should work fine. For the grids, I think its fine if it has N=0, ie, if you just want to compute something at a single point.

Right, but if length is computed with a N=0 grid, it will be completely incorrect right?

@f0uriest
Copy link
Member Author

f0uriest commented Aug 4, 2023

Ah OK i see what you mean. Like how we always compute volume on a quadrature grid and profiles on a linear grid, then seed the data dictionary with those values. I can do something similar for curves/surfaces

@f0uriest f0uriest requested a review from dpanici August 7, 2023 18:14
@f0uriest f0uriest merged commit 4a34ffc into master Aug 8, 2023
@f0uriest f0uriest deleted the rc/compute_geometry branch August 8, 2023 18:36
unalmis added a commit that referenced this pull request Aug 9, 2023
- Switch test marked regression to unit for same reason as
  git commit 44f2877
- Remove unneded test for limits with fix iota
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

move coil compute stuff to base class, add universal compute method
3 participants