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

Surfaces can compute more than they should #1127

Open
unalmis opened this issue Jul 12, 2024 · 2 comments
Open

Surfaces can compute more than they should #1127

unalmis opened this issue Jul 12, 2024 · 2 comments
Labels
bug Something isn't working documentation Add documentation or better warnings etc. good first issue Good for newcomers P3 Highest Priority, someone is/should be actively working on this

Comments

@unalmis
Copy link
Collaborator

unalmis commented Jul 12, 2024

We have things in the list of variables that we say we can compute on certain objects, but shouldn't be able. One could be misled that a quantity not typically associated with their surface was computed somehow by integrating on the boundary, or just by sampling the given surface.

If a quantity cannot be computed on an object, we should just not have a compute function for that object instead of setting dummy zero values.

Stetting dummy values causes bugs. For example n_rho defined as $\nabla \rho / \Vert \nabla \rho \Vert$ should not be defined on a ZernikeRZToroidalSection. But we say it is, and give it a dummy value of 0. Now, the area of that cross-section is well-defined, however we compute that area using n_rho, and since that dependency is a dummy for this surface, it shouldn't give the correct value.

@unalmis unalmis added documentation Add documentation or better warnings etc. good first issue Good for newcomers not started bug Something isn't working and removed not in progress labels Jul 12, 2024
@unalmis
Copy link
Collaborator Author

unalmis commented Jul 20, 2024

Remove @pytest.mark.xfail(reason="GitHub issue 1127.") from tests/test_configuration.py::TestGetSurfaces::test_get_zeta_surface once resolved.

@dpanici
Copy link
Collaborator

dpanici commented Aug 13, 2024

Fix for this is to remove the Surface level implementations of the compute quantities in question, and implement them only for the subclasses that actually make sense

@dpanici dpanici added easy Short and simple to code or review and removed easy Short and simple to code or review labels Aug 13, 2024
@unalmis unalmis mentioned this issue Aug 20, 2024
8 tasks
@dpanici dpanici added the P3 Highest Priority, someone is/should be actively working on this label Nov 11, 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 documentation Add documentation or better warnings etc. good first issue Good for newcomers P3 Highest Priority, someone is/should be actively working on this
Projects
None yet
Development

No branches or pull requests

2 participants