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

Magnet length branch #216

Merged
merged 12 commits into from
Dec 9, 2024
Merged

Conversation

phys-cgarnier
Copy link
Collaborator

Updated magnet length branch.

Copy link
Member

@nneveu nneveu left a comment

Choose a reason for hiding this comment

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

I'm approving because the yaml files look ok. We should rethink this test and not sure followed all the metadata logic.

)
return {}
if magnet_names and method:
# Add any additional metadata fields here
Copy link
Member

Choose a reason for hiding this comment

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

What would method be here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Method is the method you want to pass for parsing the lcls_elements.csv file.
Since generate.py had already parsed the csv file for other elements I figured we didn't need to duplicate the code here. Idk what your thoughts on this are.

# self.magnet.l_eff = "a"
# some trouble with this.... need to change config later. its not forcing
# revalidation after reassignment since type is optional I believe.
self.magnet.l_eff = 1.0
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't have to prevent merging, but I think some of these tests can be restructured. We shouldn't assign then check the value? Maybe we can compare object value vs. what's in the csv table instead.

Copy link
Member

Choose a reason for hiding this comment

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

Can you write this up in an issue? @phys-cgarnier

)
else:
elements = self._filter_elements_by_fields(
required_fields=self._required_fields
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm not following the logic here? Maybe a Friday thing. So there is no case where something in self._required_fields would be in required_fields?

@phys-cgarnier phys-cgarnier merged commit 7683d6f into slaclab:main Dec 9, 2024
3 checks passed
@phys-cgarnier phys-cgarnier deleted the magnet_length_branch branch January 6, 2025 19:44
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.

2 participants