Skip to content

En/review eg/fields surfaces objective functions mod#40

Open
eduardolneto wants to merge 6 commits into
eg/analysisfrom
en/review_eg/fields_surfaces_objective_functions_mod
Open

En/review eg/fields surfaces objective functions mod#40
eduardolneto wants to merge 6 commits into
eg/analysisfrom
en/review_eg/fields_surfaces_objective_functions_mod

Conversation

@eduardolneto

Copy link
Copy Markdown
Contributor

Small changes on fields.py class near_axis to work as a dependecy of custom_losses. Refactoring loss functions in objective_functions.py to meet the standard of new custom losses implementation. Adding surface-coil disstance, linking number and coils forces loss functions. Mostly changes to fields.py and objective_functions.py

…custom_losses. Refactoring loss functions in objective_functions.py to meet the standard of new custom losses implementation. Adding surface-coil disstance, linking number and coils forces loss functions
@eduardolneto eduardolneto changed the base branch from main to eg/analysis June 27, 2026 14:30
Comment thread essos/surfaces.py

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove these changes. Removing spaces is fine, but it may cause some merge issues with other people's PRs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this is fine. The space was removed and added again, so there are virtually no chnages? This also looks the same as the file in eg/analysis branch to me.

Comment thread essos/fields.py
def dofs(self, new_dofs):
self._dofs = jnp.array(new_dofs)
# Ensure dofs are always float for JAX autodiff compatibility
self._dofs = jnp.array(new_dofs, dtype=float)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should not be needed...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was related with the way near axis is constructed. Some values are technically integers at the near-axis class level. This may not be needed now. However, if we are going to use a jax pyqsc version that may actually be necessary again because I think the original version may output some of the quantities as integers. I would keep it for now and change it later.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Then I think it should be changed in JAX PyQSC and not here. But it's ok to keep now.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like that this was simplified, but I still need to check the losses more thoroughly. An example where they are used would be useful for that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@EstevaoMGomes In the examples, I kept similar local loss functions because that was the exisiting structure there. I could change them to always improt from this file. However, adding those examples to this PR would imply that I would need to add pleanty of file modifications for the examples. Which is something I thought you did not want previously.

@rogeriojorge rogeriojorge left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. objective_functions.py:222, 234, and 243: loss_BdotN(), loss_BdotN_only(), and loss_BdotN_only_constraint() still call field_from_dofs(...), but this PR removes field_from_dofs() earlier in the file. These functions will now raise NameError. You could make a small change of these losses to accept a field directly like the new custom-loss style.

  2. I didn't test locally, but seems that objective_functions.py:81, 92, 114, 135, and 137: particle-loss functions use xyz[:,0], xyz[:,1], xyz[:,2], which selects timesteps, not coordinates, since xyz has shape (n_particles, n_times, 3). Should this be xyz[:, :, 0], xyz[:, :, 1], xyz[:, :, 2]? Otherwise jnp.diff(..., axis=1) is not differentiating the intended time series.

  3. objective_functions.py:155 and 170: loss_particle_Br() and loss_particle_iota() accept a timestep argument but still pass timestep=1.e-8 to Tracing. I’d replace those with timestep=timestep, otherwise the user-provided value is ignored.

Minor things

objective_functions.py:460-505, loss_lorentz_force_coils(): block_size is in the signature, static args, and docstring, but it is not used. I’d either remove it from this function for now, or actually use it to block the mutual-field calculation. Also n_points on line 465 is unused.

new_nearaxis_from_x_and_old_nearaxis and other imports like lax might be unused and could be deleted

@eduardolneto

Copy link
Copy Markdown
Contributor Author

I have addressed all comments. Added an exampel to test surface-distance distance-distance, linkingnumber and coisl forces. There are some incositencies with coils.py. However, these shjould only be addressed in a PR once all the current PR #40 #38 #39 #37 #36 are merged. Because it will need to use the updated coils.py file.

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.

3 participants