OpenDrift / opendrift

Open source framework for ocean trajectory modelling
https://opendrift.github.io
GNU General Public License v2.0
247 stars 120 forks source link

split env #1164

Closed gauteh closed 11 months ago

gauteh commented 1 year ago
knutfrode commented 1 year ago

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 commented 1 year ago

I might have forgotten to move it from base model.

knutfrode commented 1 year ago

I might have forgotten to move it from base model.

Ok, then I can do it

knutfrode commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Ok!

gauteh commented 1 year ago

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 commented 1 year ago

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 commented 12 months ago

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.

knutfrode commented 11 months ago

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

gauteh commented 11 months ago

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 commented 11 months ago

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

gauteh commented 11 months ago

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 commented 11 months ago

Should we merge?

knutfrode commented 11 months ago

Yes, ok with me