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

SWAN Boundary #66

Merged
merged 37 commits into from
Apr 18, 2024
Merged

SWAN Boundary #66

merged 37 commits into from
Apr 18, 2024

Conversation

rafa-guedes
Copy link
Collaborator

Implement data objects to generate different types of swan boundary.

  • BOUNDNEST1 (previously available but had a few changes and improvements here)
  • BOUNDSPEC
    • SIDE and SEGMENT both supported
    • CONstant supported
    • FILE supported
      • TPAR supported
      • SPEC2D supported

Example notebooks showing usage for all the new classes are defined here.

Copy link
Collaborator

@benjaminleighton benjaminleighton left a comment

Choose a reason for hiding this comment

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

Hi @rafa-guedes. Cagil and I have both had a look at the notebooks in some detail and it looks good to us. Cagil commented that in some cases in rotated domains if the XY boundary points don't correspond to the boundary line then there is a risk that SWAN may threshold them out. We weren't clear on the exact circumstances were this occurs though so perhaps with your implementation it won't happen but it's something to watch out for. To avoid it Cagil has previously used an IJ specification. I have aspirations to integration variable boundary forcing into our SWAN domains to test in context but haven't been able to do that yet.

@rafa-guedes
Copy link
Collaborator Author

Thanks @benjaminleighton ,

there is still some work to do indeed for this to entirely support rotated domains. In the SIDE type boundary for example, SWAN applies the boundary to the side whose normal has the nearest direction to the specified side. However these objects use coordinates from the specific side in the grid array to interpolate the boundary data.

The IJ specification will not be difficult to implement, the object will look similar to SegmentXY but we will need to translate coordinates into grid nodes.

@rafa-guedes
Copy link
Collaborator Author

Closes #37

@rafa-guedes
Copy link
Collaborator Author

@tomdurrant I have synced main here and fixed the issue with filter_time, this should be ready to be reviewed.

@benjaminleighton
Copy link
Collaborator

@rafa-guedes I'm getting some issues model.generate() is working alright but round tripping model.model_dump(mode='json') back to model = ModelRun(**model_as_json) is throwing a bunch of errors

ValidationError: 7 validation errors for ModelRun
config.swanconfig.boundary.boundary_interface.kind.function-after[assert_has_wavespectra_accessor(), Boundnest1].data_type
Input should be 'boundary' [type=literal_error, input_value='data_boundary', input_type=str]
For further information visit https://errors.pydantic.dev/2.5/v/literal_error
config.swanconfig.boundary.boundary_interface.kind.function-after[assert_has_wavespectra_accessor(), BoundspecSide].model_type
Input should be 'boundspecside' or 'BOUNDSPECSIDE' [type=literal_error, input_value='boundnest1', input_type=str]
For further information visit https://errors.pydantic.dev/2.5/v/literal_error
config.swanconfig.boundary.boundary_interface.kind.function-after[assert_has_wavespectra_accessor(), BoundspecSide].data_type
Input should be 'boundary' [type=literal_error, input_value='data_boundary', input_type=str]
For further information visit https://errors.pydantic.dev/2.5/v/literal_error
config.swanconfig.boundary.boundary_interface.kind.function-after[assert_has_wavespectra_accessor(), BoundspecSide].location
Field required [type=missing, input_value={'model_type': 'boundnest...n', 'rectangle': 'open'}, input_type=dict]
For further information visit https://errors.pydantic.dev/2.5/v/missing
config.swanconfig.boundary.boundary_interface.kind.function-after[assert_has_wavespectra_accessor(), BoundspecSegmentXY].model_type
Input should be 'boundspecside' or 'BOUNDSPECSIDE' [type=literal_error, input_value='boundnest1', input_type=str]
For further information visit https://errors.pydantic.dev/2.5/v/literal_error
config.swanconfig.boundary.boundary_interface.kind.function-after[assert_has_wavespectra_accessor(), BoundspecSegmentXY].data_type
Input should be 'boundary' [type=literal_error, input_value='data_boundary', input_type=str]
For further information visit https://errors.pydantic.dev/2.5/v/literal_error
config.swanconfig.boundary.boundary_interface.kind.function-after[assert_has_wavespectra_accessor(), BoundspecSegmentXY].location
Field required [type=missing, input_value={'model_type': 'boundnest...n', 'rectangle': 'open'}, input_type=dict]
For further information visit https://errors.pydantic.dev/2.5/v/missing

@rafa-guedes
Copy link
Collaborator Author

@benjaminleighton I'll have a look at this. Has this worked on the existing version of the code in main as well or is it specific to this branch?

@benjaminleighton
Copy link
Collaborator

@rafa-guedes I haven't tried against main because I think I'm heavily using components of this branch in the setup. Most of the errors look like they might be related to new boundary spec stuff. This could be something todo with my install as I"m getting warnings too like

////lib/python3.10/site-packages/pydantic/main.py:308: UserWarning: Pydantic serializer warnings:
Expected Union[BaseConfig, SwanConfig, definition-ref, definition-ref, SchismCSIROConfig] but got SwanConfigComponents - serialized value may not be as expected
Expected Union[definition-ref, definition-ref, definition-ref, definition-ref, definition-ref, definition-ref, definition-ref, definition-ref, definition-ref, definition-ref] but got BoundaryInterface - serialized value may not be as expected
Expected Union[Boundnest1, BoundspecSide, BoundspecSegmentXY] but got Boundnest1 - serialized value may not be as expected
return self.pydantic_serializer.to_python(

the last one of those saying Boundnest1 doesn't match Boundnest1 doesn't make much sense.

@benjaminleighton
Copy link
Collaborator

@rafa-guedes I'm trying and failing to replicate my error in a clean github workspaces enviornment so I think this maybe an issue with my local installation.

@benjaminleighton
Copy link
Collaborator

Sorry @rafa-guedes, Actually I think I've replicated it in a slightly modified example_procedural_boundspec.ipynb see attached
example_procedural_boundspec.zip. Changing the boundary forcing to use Boundnest1 like:

from rompy.swan.boundary import BoundspecSide, BoundspecSegmentXY, Boundnest1
from rompy.swan.interface import BoundaryInterface
from rompy.swan.subcomponents.boundary import SIDE, SIDES
from rompy.core.data import SourceFile

bndsource = SourceFile(
    uri=DATADIR / "aus-20230101.nc",
    kwargs=dict(engine="netcdf4"),
)
location = SIDES(
    sides=[
        SIDE(side="south", direction="clockwise"),
        SIDE(side="west", direction="clockwise"),
    ],
)

bnd = Boundnest1(
    id="westaus",
    source=bndsource,
)

boundary_segment = BoundaryInterface(kind=bnd)
boundary_segment

then later after

from rompy.model import ModelRun
from rompy.core.time import TimeRange

start, end = era5.time.to_index()[[0, -1]]

run = ModelRun(
    run_id="run1",
    period=TimeRange(start=start, end=end, interval="1h"),
    output_dir=str(workdir),
    config=config,
)

rundir = run()

trying

SwanConfigComponents(**config.model_dump())

@rafa-guedes
Copy link
Collaborator Author

@benjaminleighton thanks for the reproducible example. This should hopefully be fixed with eb43e42, there was some change in a core base class that was the root cause all those validation errors.

Out of curiosity, why are you instantiating the Config class using the model_dump of an existing Config instance? This approach may fail with some other objects. For example, if we try to instantiate ModelRun with the dump of an existing ModelRun instance, it fails because the TimeRange object requires only two of start, end or duration to be provided, but the instance will have had all of those defined.

@tomdurrant tomdurrant merged commit 36a520b into main Apr 18, 2024
@benjaminleighton
Copy link
Collaborator

@rafa-guedes I was thinking that this would be a good way to serialize the whole model configuration, allow for editing it and load it back in.

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.

3 participants