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

eq.compute doesn't correctly override grids to compute quantities that require evaluating functions along field lines #904

Closed
rahulgaur104 opened this issue Feb 25, 2024 · 6 comments · Fixed by #1024
Assignees
Labels
bug Something isn't working

Comments

@rahulgaur104
Copy link
Collaborator

In the compute function in equilibrium.py if the user provides a grid, the function should set override_grid = False.

https://github.com/PlasmaControl/DESC/blob/f1689c85c022d5e8073325b7c057f6ea253923b4/desc/equilibrium/equilibrium.py#L794C1-L800C14

However, override_grid still stays True. This is a simple fix.

@unalmis
Copy link
Collaborator

unalmis commented Feb 25, 2024

This logic is intentional, see #440

@f0uriest
Copy link
Member

See also #688 and related PR

@rahulgaur104
Copy link
Collaborator Author

rahulgaur104 commented Feb 26, 2024

Well, it definitely throws an error on the ballooning branch when I create a separate grid before calculating the growth rate using eq.compute.

I get assertion error and see this code

            data1dr = compute_fun(
                self,
                dep1dr,
                params=params,
                transforms=get_transforms(dep1dr, obj=self, grid=grid1dr, **kwargs),
                profiles=get_profiles(dep1dr, obj=self, grid=grid1dr),
                data=None,
                **kwargs,
            )
            # need to make this data broadcast with the data on the original grid
            data1dr = {
                key: grid.expand(
                    grid1dr.compress(val, surface_label="rho"), surface_label="rho"
                )
                for key, val in data1dr.items()
                if key in dep1dr
            }

@f0uriest
Copy link
Member

It sounds like you might just need to pass override_grid=False when you call eq.compute if you have some custom grid you want to use and don't care about averages etc (because you're precomputing those on a different grid). That's the purpose of the override_grid option.

If you can post some code that reproduces the issue I can look into it more.

@rahulgaur104
Copy link
Collaborator Author

rahulgaur104 commented Feb 26, 2024

Yeah, with override_grid=False, it worked fine. I was just thinking that it could be a potential issue for people doing calculations on a custom grid. It also only becomes an issue when I do this calculation in a jupyter notebook.

Ok, I guess we can close this for now.

@unalmis unalmis reopened this Apr 11, 2024
@unalmis
Copy link
Collaborator

unalmis commented Apr 26, 2024

At some point, can you show where you have needed to use override_grid=False so that I can check your code? I believe it indicates that your code has a bug. Edit: actually this isn't always true so nevermind.

For example, I recently fixed a bug in bounce branch where this was an issue with an inconsistent use of normalization between the toroidal flux label and desc's radial label. (This can lead to computations being done across different flux surfaces than expected). In this case, setting this to override_grid=False will temporarily supress the error, but it causes bugs that will pop up elsewhere.

@unalmis unalmis closed this as completed Apr 27, 2024
unalmis added a commit that referenced this issue Jun 2, 2024
unalmis added a commit that referenced this issue Jun 2, 2024
@unalmis unalmis reopened this Jun 3, 2024
@unalmis unalmis changed the title The variable override_grid should be set to False when the user provides a grid eq.compute doesn't correctly override grids to compute quantities that require evaluating functions along field lines Jun 25, 2024
@unalmis unalmis added the bug Something isn't working label Jul 1, 2024
@unalmis unalmis self-assigned this Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants