Skip to content

Conversation

@ax3l
Copy link
Member

@ax3l ax3l commented Oct 29, 2025

In preparation of #38, refactor MagneticMultipoleParameters.

  • remove code duplication and hard-coded string accesses
  • more detailed error messages
  • add support for Kn and Ks and length-integrated ...L parameters

- remove code duplication and hard-coded string accesses
- more detailed error messages
- add support for `Kn` and `Ks` and length-integrated `...L`
  parameters
@ax3l ax3l requested review from EZoni and cemitch99 October 29, 2025 19:25
@ax3l ax3l added the element parameters element parameters (properties, attributes) label Oct 29, 2025
Comment on lines -21 to +44
def validate(cls, values: Dict[str, Any]) -> Dict[str, Any]:
# loop over all attributes
@classmethod
def validate(cls, values: dict[str, Any]) -> dict[str, Any]:
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference between using dict and Dict (from typing) and why is it better to use dict here?

Copy link
Member Author

@ax3l ax3l Oct 29, 2025

Choose a reason for hiding this comment

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

dict is a Python intrinsic and needs no import. Thus, better.

Better generally is:

  1. intrinsic of the language
  2. stdlib package (typing, etc.)
  3. pydantic

In terms of compatibility (i.e., more tools understand intrinsics than stdlib than 3rd part libs) and performance (e.g., less imports is better).

@EZoni
Copy link
Member

EZoni commented Oct 29, 2025

Note that everywhere we say "must be a positive integer (>=0)..." or "must be a valid positive integer (>=0)", but I think this is typically referred to as a "non-negative integer", given that 0 isn't positive. I would personally rephrase all instances with "must be a non-negative integer".

@ax3l
Copy link
Member Author

ax3l commented Oct 29, 2025

Yeah, I replaced non-negative integer because I thought "positive integer" reads easier (and there is the extra in parenthesis for the people that know "strictly positive integer" would refer to not zero, which is not the case here).

In essence, I tried to avoid a double negative for readability :)

@ax3l ax3l requested a review from EZoni October 29, 2025 22:08
Co-authored-by: Edoardo Zoni <ezoni@lbl.gov>
@EZoni EZoni merged commit 2ddf2a7 into pals-project:main Oct 30, 2025
5 checks passed
@ax3l ax3l deleted the topic-refactor-magnetic-multipole-param branch November 1, 2025 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

element parameters element parameters (properties, attributes)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants