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

Python: ShortRF Properties #646

Merged
merged 1 commit into from
Jul 18, 2024
Merged

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Jul 18, 2024

Add element properties for ShortRF, so it can be modified after variable creation, too.

@ax3l ax3l added component: elements Elements/external fields component: python Python bindings labels Jul 18, 2024
@ax3l ax3l requested a review from cemitch99 July 18, 2024 15:52
Add element properties for `ShortRF`, so it can be modified after
variable creation, too.
@ax3l
Copy link
Member Author

ax3l commented Jul 18, 2024

@EZoni volunteered to complete the other leftover elements in a separate PR 🙏

@ax3l ax3l added this to the Sirepo (FY24 SBIR) milestone Jul 18, 2024
Copy link
Member

@cemitch99 cemitch99 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks.

@ax3l ax3l enabled auto-merge (squash) July 18, 2024 16:34
Copy link
Member

@cemitch99 cemitch99 left a comment

Choose a reason for hiding this comment

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

Looks great.

@ax3l ax3l merged commit 9fed70e into ECP-WarpX:development Jul 18, 2024
15 of 16 checks passed
Comment on lines +977 to +984
.def_property("freq",
[](ShortRF & short_rf) { return short_rf.m_freq; },
[](ShortRF & short_rf, int freq) { short_rf.m_freq = freq; },
"RF frequency in Hz"
)
.def_property("phase",
[](ShortRF & short_rf) { return short_rf.m_phase; },
[](ShortRF & short_rf, int phase) { short_rf.m_phase = phase; },
Copy link
Member

Choose a reason for hiding this comment

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

Actually I just noticed this, should freq and phase be amrex::ParticleReal too, as opposed to int? If so, I can fix it in the follow-up PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for catching this! Those should definitely be amrex::ParticleReal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooops, yes please. Copy pasted this, dang!

Copy link
Member

Choose a reason for hiding this comment

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

@ax3l
I will fix it in the upcoming PR. Question, which I guess is pybind11-related: why does the compiler not complain about/catch this discrepancy between types?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am surprised as well, should issue a narrowing warning.
https://godbolt.org/z/f6bE3xPPE

Thx for the fix in #647

@ax3l ax3l deleted the py-prop-shortrf branch July 18, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: elements Elements/external fields component: python Python bindings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants