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

Add TaggedVariant, parsable similarly to a factory #6096

Merged
merged 2 commits into from
Jul 12, 2024

Conversation

wthrowe
Copy link
Member

@wthrowe wthrowe commented Jun 17, 2024

I have mixed feelings on the visit interface. The version here follows std::visit and allows multiple TaggedVariants to be passed to a single call, but this makes reporting the tag awkward and, in particular, prevents implicit conversions between reference types in the visitor arguments. An alternative would be to restrict to the (I assume vastly most common) case of a single TaggedVariant and pass the tag and value as separate arguments, which would fix all the conversion trouble. Opinions are welcome.

Ping @knelli2

Proposed changes

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

@nilsdeppe
Copy link
Member

For what it's worth, I added multi-container gets https://github.com/sxs-collaboration/spectre/blob/develop/src/DataStructures/TaggedContainers.hpp at some point because, while not the most common case, we did need it

@wthrowe
Copy link
Member Author

wthrowe commented Jun 17, 2024

Apparently the cppreference compiler support tables are wrong or incomplete, so I'll have to figure out what compiler versions constexpr support works in. Clang 17 and gcc 13 are known working. gcc 9 was expected to fail, so it does runtime tests instead of static assertions, so I'll switch other versions to that once I figure out what they are.

@wthrowe
Copy link
Member Author

wthrowe commented Jun 18, 2024

The first working gcc version is 12. I can't easily test clang because clang 15 can't compile DomainCreators and my distro's clang 16 package won't install for some reason I don't feel like debugging. I'm guessing the changes happened at the same time as constexpr std::vector, which was gcc 12 and clang 15, so that's what I went with there.

Copy link
Contributor

@knelli2 knelli2 left a comment

Choose a reason for hiding this comment

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

Thanks for adding this Will! I think this will work very well. As for the visit interface. I think you can restrict it to only acting on a single TaggedVariant. I imagine this will basically only be used for option stuff in which case we'll only need to visit the tags in one variant. But I don't have super strong feelings either way.

src/DataStructures/TaggedVariant.hpp Outdated Show resolved Hide resolved
src/DataStructures/TaggedVariant.hpp Show resolved Hide resolved
Comment on lines +145 to +152
/// @{
static constexpr Options::String help = "One of multiple options";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way that we can specify this string? Possibly from a template parameter somehow?

I've tested this PR out on some domain creator FoT changes I'm making, and it works really well! I think this will make input files much clearer. The only thing is that when a typo is made in the input file, this help string isn't very helpful. E.g. this was (part of) the error message I got

In string:
  While parsing option TestCreationOpt:
  While creating a TranslationMap:
  While parsing option InitialValues:
  While creating a TaggedVariant:
  At line 3 column 5:
  Cannot decide between alternative options.
  
==== Parsing the option string:
  FromVolFil:
    H5Filename: BbhVolume0.h5
    SubfileName: VolumeData
    Time: 0.0
  
==== Description of expected options:
  One of multiple options
  
Options:
    EITHER
      Explicit:
        Something useful here
  
  OR
      FromVolFile:
        Something useful here

If under the Description of expected options part I was able to use the help string for the actual option this was creating, that'd be much clearer I think (i.e. Initial values for the translation map, its velocity and acceleration.)

Copy link
Member Author

Choose a reason for hiding this comment

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

There's nowhere to put a template parameter, unfortunately.

What might be good in general is if there was an error when creating a class it also displayed the help from the tag, but currently the code generating the error message doesn't know that information.

We could add the help test for each option after the "While parsing option" lines in the "backtrace" pretty easily. It's not the most visible place, but at least it would be there. Example from ExportCoordinates:

In /home/wthrowe/spectre/tests/InputFiles/ExportCoordinates/Input1D.yaml
While parsing option DomainCreator
  The domain to create initially
While creating a unique_ptr
While operating factory for DomainCreator
While creating a Interval
While parsing option TimeDependence
  The time dependence of the moving mesh domain.
While creating a unique_ptr
While operating factory for TimeDependence
While creating a UniformTranslation
At line 48 column 9:
Option 'VelocityX' is not a valid option.

==== Parsing the option string:
InitialTime: 0.0
VelocityX: [0.5]

==== Description of expected options:
A spatially uniform translation initialized with a constant velocity.

Options:
  InitialTime:
    type=double
    The initial time of the functions of time

  Velocity:
    type=[double x1]
    The velocity of the map.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wthrowe I'm ok forgoing this change for the time being. I think the benefit to the input file syntax is enough for the moment.

Copy link
Contributor

@knelli2 knelli2 left a comment

Choose a reason for hiding this comment

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

Fixup looks good. Go ahead and squash

@knelli2 knelli2 enabled auto-merge July 12, 2024 16:18
@knelli2 knelli2 added the auto-merge GitHub's auto-merge has been enabled for this PR. label Jul 12, 2024
@knelli2 knelli2 merged commit f88cc3a into sxs-collaboration:develop Jul 12, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge GitHub's auto-merge has been enabled for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants