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

Return physical constants object from opacities #52

Merged
merged 8 commits into from
Sep 28, 2024
Merged

Conversation

brryan
Copy link
Collaborator

@brryan brryan commented Sep 26, 2024

It would be nice to see the physical constants being used by singularity-opac in downstream codes for consistency. This PR provides a way to do that, although we lose the constexprness of the constants with this approach (templating the variant to make ReturnPhysicalConstants a static method appears to be a tricky proposition).

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

This is a nice idea. Unfortunate we can't output the constants with a static method though... I feel like that's much more likely the model we'd want in, e.g., Phoebus. Are you sure you can't do static constexpr to output the object?

@brryan
Copy link
Collaborator Author

brryan commented Sep 27, 2024

This is a nice idea. Unfortunate we can't output the constants with a static method though... I feel like that's much more likely the model we'd want in, e.g., Phoebus. Are you sure you can't do static constexpr to output the object?

The issue I ultimately couldn't avoid was having a variant method that's like

PORTABLE_INLINE_FUNCTION
auto GetPhysicalConstants() const {
  return mpark::visit[](... opac.GetPhysicalConstants();
}

because type deduction for the return value fails because it'll necessarily be different PhysicalConstants types depending on the unit system.

We may want to leave phoebus as-is for now, this change was partially motivated to support made-up physical constants being used for some shock tube test problems, but this approach also lets one at least check the singularity-opac constants for consistency with a downstream code which isn't a performance-critical operation.

My workaround is to have a RuntimePhysicalConstants (constbut notstatic constexprmembers) that is constructed with a constructor templated on aPhysicalConstants object.

@Yurlungur
Copy link
Collaborator

Yeah I think your workaround is reasonable. I wonder if an alternative workaround is if the unit system modifier lives on top of the variant, so the return type is known and no visit is required? I.e., an individual opacity doesn't own the unit system, the variant itself does?

@brryan
Copy link
Collaborator Author

brryan commented Sep 28, 2024

Yeah I think your workaround is reasonable. I wonder if an alternative workaround is if the unit system modifier lives on top of the variant, so the return type is known and no visit is required? I.e., an individual opacity doesn't own the unit system, the variant itself does?

I think the issue with doing that is that in a downstream code youll then have to template on the unit system, so you'd have to either have one unit system at compile time or template your whole code on the unit system. Stop me if I'm not seeing a way around that though.

@Yurlungur
Copy link
Collaborator

Ah yeah you're right. Let's leave it then.

@brryan brryan merged commit e429a53 into main Sep 28, 2024
1 check passed
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