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

split env #1164

Merged
merged 21 commits into from
Oct 20, 2023
Merged

split env #1164

merged 21 commits into from
Oct 20, 2023

Conversation

gauteh
Copy link
Member

@gauteh gauteh commented Sep 26, 2023

  • environment: set up environment class
  • basemodel: move reader logic to env

@knutfrode
Copy link
Collaborator

knutfrode commented Sep 28, 2023

One test is complaining about missing method o.get_variables_along_trajectory
I cannot find this method anymore?

This is how it was, in basemodel.py:


    def get_variables_along_trajectory(self, variables, lons, lats, times):
        data = {'time': times, 'lon': lons, 'lat': lats}
        for var in variables:
            data[var] = np.zeros(len(times))
        for i, time in enumerate(times):
            self.time = time
            d = self.get_environment(lon=np.atleast_1d(lons[i]),
                                     lat=np.atleast_1d(lats[i]),
                                     z=np.atleast_1d(0),
                                     time=time,
                                     variables=variables,
                                     profiles=None)
            for var in variables:
                data[var][i] = d[0][var][0]

        return data

@gauteh
Copy link
Member Author

gauteh commented Sep 28, 2023

I might have forgotten to move it from base model.

@knutfrode
Copy link
Collaborator

I might have forgotten to move it from base model.

Ok, then I can do it

@knutfrode
Copy link
Collaborator

knutfrode commented Oct 5, 2023

Det ser ut som det er ett problem som gjenstår, at man ikkje kan be om data (feks ved seeding med z=seafloor) før Environment er finalized

Kunne det være ein ide at get_environment får ein ekstra parameter allow_not_finalized som kan settes til True for spesialtilfellene der man feks vil hente ut seafloor_depth før simuleringen starter (i.e. ved seeding)?

@gauteh
Copy link
Member Author

gauteh commented Oct 5, 2023

Da vil feks fallback med seafloor_depth ikkje fungere. Er det ønskelig? Kanskje bedre å finalize før seeding. Ellers trur eg det er lett at det kan oppstå forvirring, eller vi kan få forskjellige resultat.

Det gir jo egentlig ikkje meining å kalle seed med seafloor uten at lesere som gir eit havdjup er initialisert.

@knutfrode
Copy link
Collaborator

knutfrode commented Oct 5, 2023

Til nå har det vært mulig å seede før man oppretter environment. Men kanskje rekkefølgen kan/bør låses til

o.mode = enum[config, ready, run, result]

  1. opprette objekt -> === config mode ===

  2. legge til environment (skjønt kan være lazy)

  3. config

  4. seeding -> === ready mode mode ===

  5. run -> === run mode ===

Dvs, trenger kanskje ikkje fastlåse dette, men første kall til seed_elements der z e.a. , kunne finalize Environmentet

@gauteh
Copy link
Member Author

gauteh commented Oct 5, 2023

Ja, einig. Det vil gjere det lettere for oss også. Det ligner litt på Init/Simulation/Result state som eg prøvde på, men vi kan gjere ein mellomløysing der vi tracker state internt. Bakdelen er at vi må sjekke i kvar metode at vi er i riktig state, men vi kan kanskje fikse det med dekoratorer på ein god måte.

@knutfrode
Copy link
Collaborator

Ok, eg kan prøve å lage et første, fungerende eksempel. Men da kanskje uten dekoratorer i første omgang, siden eg ikkje har brukt det så masse.

@gauteh
Copy link
Member Author

gauteh commented Oct 5, 2023

Ok!

@gauteh
Copy link
Member Author

gauteh commented Oct 7, 2023

I created a decorator to ensure the correct mode (or state):

    @require_mode(mode=Mode.Config, error='Cannot set config after elements have been seeded')
    @functools.wraps(Configurable.set_config)
    def set_config(self, *args, **kwargs):

I wrapped set_config in BaseModel so that Configurable doesn't need to know about modes/states. The functools.wraps is just to forward docstring and method location from Configurable, so not necessary, but gives better error messages.

@knutfrode
Copy link
Collaborator

I created a decorator to ensure the correct mode (or state):

    @require_mode(mode=Mode.Config, error='Cannot set config after elements have been seeded')
    @functools.wraps(Configurable.set_config)
    def set_config(self, *args, **kwargs):

I wrapped set_config in BaseModel so that Configurable doesn't need to know about modes/states. The functools.wraps is just to forward docstring and method location from Configurable, so not necessary, but gives better error messages.

Very good. I had moved Modes to opendrift/init, so that it could also be imported by Configurable.
But this seems more elegant.

@knutfrode
Copy link
Collaborator

Note to self:
seed_cone stores some metadata through config values (in test_oillibrary.py::test_seed_metadata_output), but this is not anymore allowed after seeding. A workaround is needed here.

… a cached dict, but must be retrieved from config. Updated all examples and tests to seed elements after config and readers
@knutfrode
Copy link
Collaborator

Finally!
Should we then merge into master, and make 1.11.0 with updated changelog?

@gauteh
Copy link
Member Author

gauteh commented Oct 19, 2023

Nice! Good work! I can test a bit tomorrow! I wanted to put in some more checks in the decorator. Maybe we should let it be in master for testing a bit before releasing?

@knutfrode
Copy link
Collaborator

Yes, that is fine. Everything works now, but that does not mean that everything is beautiful.

@gauteh
Copy link
Member Author

gauteh commented Oct 20, 2023

I removed the reset() method, to be sure that everything is pristine and there are no leftovers the only way is to start to scratch.

@gauteh
Copy link
Member Author

gauteh commented Oct 20, 2023

Should we merge?

@knutfrode
Copy link
Collaborator

Yes, ok with me

@gauteh gauteh merged commit 3fb6c31 into OpenDrift:master Oct 20, 2023
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