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.

Sign up for GitHub

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

Use hardcoded filenames. #815

Merged
merged 9 commits into from
Nov 24, 2023
Merged

Use hardcoded filenames. #815

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2c07a1b
Use hardcoded filenames.
evetion Nov 21, 2023
8433231
Fix results paths.
evetion Nov 23, 2023
ca869bb
Merge main.
evetion Nov 23, 2023
4d434a9
Adapt `Config` struct
Hofer-Julian Nov 23, 2023
93b02d6
Address review comments
Hofer-Julian Nov 24, 2023
f864590
Move file name consts into tuple
Hofer-Julian Nov 24, 2023
f87adab
Update core/src/config.jl
Hofer-Julian Nov 24, 2023
594f35c
Rename `relative_dir` to `dir` and use `Base.dirname`
Hofer-Julian Nov 24, 2023
3fb911d
Adapt docs.toml
Hofer-Julian Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build/ribasim_cli/tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
def test_ribasim_cli(model_constructor, tmp_path):
model = model_constructor()
assert isinstance(model, ribasim.Model)
model.write(tmp_path)
model.write(tmp_path / "ribasim.toml")

executable = (
Path(__file__).parents[2]
Expand Down
14 changes: 7 additions & 7 deletions core/src/config.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,10 @@ end

# Separate struct, as basin clashes with nodetype
@option struct Results <: TableOption
basin::String = "results/basin.arrow"
flow::String = "results/flow.arrow"
control::String = "results/control.arrow"
allocation::String = "results/allocation.arrow"
basin::String = "basin.arrow"
flow::String = "flow.arrow"
control::String = "control.arrow"
allocation::String = "allocation.arrow"
Copy link
Contributor

Choose a reason for hiding this comment

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

To fit #799 (comment)

Those fields need to be removed as far as I can see

Copy link
Member Author

Choose a reason for hiding this comment

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

These fields need to be hardcodes somewhere for core to write them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but if you hardcode them here, it will be possible to override them with the TOML right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I now always override them.

outstate::Union{String, Nothing} = nothing
compression::Compression = "zstd"
compression_level::Int = 6
Expand All @@ -134,12 +134,12 @@ end
endtime::DateTime

# optional, when Config is created from a TOML file, this is its directory
relative_dir::String = "." # ignored(!)
relative_dir::String = "."
input_dir::String = "."
results_dir::String = "."
results_dir::String = "results"
visr marked this conversation as resolved.
Show resolved Hide resolved

# input, required
database::String
database::String = "database.gpkg"

allocation::Allocation = Allocation()
solver::Solver = Solver()
Expand Down
13 changes: 4 additions & 9 deletions core/test/docs.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@ starttime = 2019-01-01 # required
endtime = 2021-01-01 # required

# input files
database = "database.gpkg" # required
input_dir = "." # optional, default "."
results_dir = "results" # optional, default "."
input_dir = "." # required, default "."
results_dir = "results" # required, default "results"

# Specific tables can also go into Arrow files rather than the database.
# For large tables this can benefit from better compressed file sizes.
Expand Down Expand Up @@ -40,9 +39,5 @@ timing = false # optional, whether to log debug timing statements

[results]
# These results files are always written
flow = "results/flow.arrow" # optional, default "results/flow.arrow"
basin = "results/basin.arrow" # optional, default "results/basin.arrow"
control = "results/control.arrow" # optional, default "results/control.arrow"
allocation = "results/allocation.arrow" # optional, default "results/allocation.arrow"
compression = "zstd" # optional, default "zstd", also supports "lz4"
compression_level = 6 # optional, default 6
compression = "zstd" # optional, default "zstd", also supports "lz4"
compression_level = 6 # optional, default 6
8 changes: 4 additions & 4 deletions docs/python/examples.ipynb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@
"outputs": [],
"source": [
"datadir = Path(\"data\")\n",
"model.write(datadir / \"basic\")"
"model.write(datadir / \"basic/ribasim.toml\")"
]
},
{
Expand Down Expand Up @@ -560,7 +560,7 @@
"metadata": {},
"outputs": [],
"source": [
"model.write(datadir / \"basic_transient\")"
"model.write(datadir / \"basic_transient/ribasim.toml\")"
]
},
{
Expand Down Expand Up @@ -952,7 +952,7 @@
"outputs": [],
"source": [
"datadir = Path(\"data\")\n",
"model.write(datadir / \"level_setpoint_with_minmax\")"
"model.write(datadir / \"level_setpoint_with_minmax/ribasim.toml\")"
]
},
{
Expand Down Expand Up @@ -1374,7 +1374,7 @@
"outputs": [],
"source": [
"datadir = Path(\"data\")\n",
"model.write(datadir / \"pid_control\")"
"model.write(datadir / \"pid_control/ribasim.toml\")"
]
},
{
Expand Down
11 changes: 2 additions & 9 deletions docs/schema/Config.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,6 @@
"format": "date-time",
"type": "string"
},
"relative_dir": {
"format": "default",
"type": "string",
"default": "."
},
"input_dir": {
"format": "default",
"type": "string",
Expand All @@ -26,7 +21,7 @@
"results_dir": {
"format": "default",
"type": "string",
"default": "."
"default": "results"
},
"database": {
"format": "default",
Expand All @@ -44,8 +39,7 @@
"$ref": "https://deltares.github.io/Ribasim/schema/Solver.schema.json",
"default": {
"algorithm": "QNDF",
"saveat": [
],
"saveat": [],
"adaptive": true,
"dt": null,
"dtmin": null,
Expand Down Expand Up @@ -170,7 +164,6 @@
"required": [
"starttime",
"endtime",
"relative_dir",
"input_dir",
"results_dir",
"database",
Expand Down
7 changes: 4 additions & 3 deletions python/ribasim/ribasim/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,10 @@ class Compression(str, Enum):


class Results(BaseModel):
basin: Path = Path("results/basin.arrow")
flow: Path = Path("results/flow.arrow")
control: Path = Path("results/control.arrow")
basin: Path = Field(default=Path("basin.arrow"), exclude=True, repr=False)
flow: Path = Field(default=Path("flow.arrow"), exclude=True, repr=False)
control: Path = Field(default=Path("control.arrow"), exclude=True, repr=False)
allocation: Path = Field(default=Path("allocation.arrow"), exclude=True, repr=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

These need to be removed too

Copy link
Member Author

Choose a reason for hiding this comment

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

I've excluded them from any exports. They are here for when we need to parse the output files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just an FYI: I just found out that TOML hates these Path objects, and complains that it cannot serialize them.

However, due to the exclude_unset logic, they aren't included unless you set them yourself. For extra confusion, if you set the path (in current main) as a string, pydantic will coerce it to a Path. Then pydantic detects it has been set, and then TOML complains that it cannot serialize it.

To be honest, together with issue #811, I'm getting really radicalized against exclude_unset.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's why there's

@field_serializer("input_dir", "results_dir") 
  def serialize_path(self, path: Path) -> str: 
      return str(path)

Right? Which fields are you trying to set?

I don't like the unset behaviour either, but that should be a separate issue/discussion.

outstate: str | None = None
compression: Compression = Compression.zstd
compression_level: int = 6
Expand Down
45 changes: 24 additions & 21 deletions python/ribasim/ribasim/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@

@classmethod
def _load(cls, filepath: Path | None) -> dict[str, Any]:
if filepath is not None:
directory = context_file_loading.get().get("directory", Path("."))
context_file_loading.get()["database"] = directory / filepath
directory = context_file_loading.get().get("directory", None)
if directory is not None:
context_file_loading.get()["database"] = directory / "database.gpkg"
return {}

@classmethod
Expand Down Expand Up @@ -113,11 +113,9 @@

update_timestep: datetime.timedelta = timedelta(seconds=86400)
The output time step of the simulation in seconds (default of 1 day)
relative_dir: Path = Path(".")
The relative directory of the input files.
input_dir: Path = Path(".")
The directory of the input files.
results_dir: Path = Path(".")
results_dir: Path = Path("results")
The directory of the results files.

network: Network
Expand Down Expand Up @@ -164,11 +162,10 @@
endtime: datetime.datetime

update_timestep: datetime.timedelta = datetime.timedelta(seconds=86400)
relative_dir: Path = Path(".")
input_dir: Path = Path(".")
results_dir: Path = Path("results")
input_dir: Path = Field(default_factory=lambda: Path("."))
results_dir: Path = Field(default_factory=lambda: Path("results"))

network: Network = Field(default_factory=Network, alias="database")
network: Network = Field(default_factory=Network, alias="database", exclude=True)
results: Results = Results()
solver: Solver = Solver()
logging: Logging = Logging()
Expand Down Expand Up @@ -201,10 +198,14 @@
def serialize_dt(self, td: datetime.timedelta) -> int:
return int(td.total_seconds())

@field_serializer("relative_dir", "input_dir", "results_dir")
@field_serializer("input_dir", "results_dir")
def serialize_path(self, path: Path) -> str:
return str(path)

def model_post_init(self, __context: Any) -> None:
# Always write dir fields
self.model_fields_set.update({"input_dir", "results_dir"})

def __repr__(self) -> str:
first = []
second = []
Expand All @@ -221,13 +222,12 @@
# Default to standard repr for now
return self.__repr__()

def _write_toml(self, directory: FilePath):
directory = Path(directory)
def _write_toml(self, fn: FilePath):
fn = Path(fn)

content = self.model_dump(exclude_unset=True, exclude_none=True, by_alias=True)
# Filter empty dicts (default Nodes)
content = dict(filter(lambda x: x[1], content.items()))
fn = directory / "ribasim.toml"
with open(fn, "wb") as f:
tomli_w.dump(content, f)
return fn
Expand Down Expand Up @@ -318,22 +318,26 @@
"""Read model from TOML file."""
return cls(filepath=filepath) # type: ignore

def write(self, directory: FilePath) -> Path:
def write(self, filepath: Path | str) -> Path:
"""
Write the contents of the model to a database and a TOML configuration file.
Write the contents of the model to disk and save it as a TOML configuration file.

If ``directory`` does not exist, it is created before writing.
If ``filepath.parent`` does not exist, it is created before writing.

Parameters
----------
directory: FilePath
filepath: FilePath ending in .toml
"""
self.validate_model()
filepath = Path(filepath)
if not filepath.suffix == ".toml":
raise ValueError(f"Filepath '{filepath}' is not a .toml file.")

Check warning on line 334 in python/ribasim/ribasim/model.py

View check run for this annotation

Codecov / codecov/patch

python/ribasim/ribasim/model.py#L334 

Added line #L334 was not covered by tests
context_file_loading.set({})
directory = Path(directory)
filepath = Path(filepath)
directory = filepath.parent
directory.mkdir(parents=True, exist_ok=True)
self._save(directory, self.input_dir)
fn = self._write_toml(directory)
fn = self._write_toml(filepath)

context_file_loading.set({})
return fn
Expand All @@ -349,7 +353,6 @@
context_file_loading.get()["directory"] = filepath.parent / config.get(
"input_dir", "."
)

return config
else:
return {}
Expand Down
6 changes: 3 additions & 3 deletions python/ribasim/tests/test_io.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def assert_equal(a, b):

def test_basic(basic, tmp_path):
model_orig = basic
model_orig.write(tmp_path / "basic")
model_orig.write(tmp_path / "basic/ribasim.toml")
model_loaded = ribasim.Model(filepath=tmp_path / "basic/ribasim.toml")

index_a = model_orig.network.node.df.index.to_numpy(int)
Expand All @@ -39,15 +39,15 @@ def test_basic(basic, tmp_path):

def test_basic_arrow(basic_arrow, tmp_path):
model_orig = basic_arrow
model_orig.write(tmp_path / "basic_arrow")
model_orig.write(tmp_path / "basic_arrow/ribasim.toml")
model_loaded = ribasim.Model(filepath=tmp_path / "basic_arrow/ribasim.toml")

assert_equal(model_orig.basin.profile.df, model_loaded.basin.profile.df)


def test_basic_transient(basic_transient, tmp_path):
model_orig = basic_transient
model_orig.write(tmp_path / "basic_transient")
model_orig.write(tmp_path / "basic_transient/ribasim.toml")
model_loaded = ribasim.Model(filepath=tmp_path / "basic_transient/ribasim.toml")

assert_equal(model_orig.network.node.df, model_loaded.network.node.df)
Expand Down
2 changes: 1 addition & 1 deletion python/ribasim/tests/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,5 +115,5 @@ def test_node_ids_unsequential(basic):

def test_tabulated_rating_curve_model(tabulated_rating_curve, tmp_path):
model_orig = tabulated_rating_curve
model_orig.write(tmp_path / "tabulated_rating_curve")
model_orig.write(tmp_path / "tabulated_rating_curve/ribasim.toml")
Model.read(tmp_path / "tabulated_rating_curve/ribasim.toml")
22 changes: 11 additions & 11 deletions python/ribasim_api/tests/test_bmi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,28 +9,28 @@


def test_initialize(libribasim, basic, tmp_path):
basic.write(tmp_path)
basic.write(tmp_path / "ribasim.toml")
config_file = str(tmp_path / "ribasim.toml")
libribasim.initialize(config_file)


def test_get_start_time(libribasim, basic, tmp_path):
basic.write(tmp_path)
basic.write(tmp_path / "ribasim.toml")
config_file = str(tmp_path / "ribasim.toml")
libribasim.initialize(config_file)
time = libribasim.get_start_time()
assert time == pytest.approx(0.0)


def test_get_current_time(libribasim, basic, tmp_path):
basic.write(tmp_path)
basic.write(tmp_path / "ribasim.toml")
config_file = str(tmp_path / "ribasim.toml")
libribasim.initialize(config_file)
assert libribasim.get_current_time() == pytest.approx(libribasim.get_start_time())


def test_get_end_time(libribasim, basic, tmp_path):
basic.write(tmp_path)
basic.write(tmp_path / "ribasim.toml")
config_file = str(tmp_path / "ribasim.toml")
libribasim.initialize(config_file)
actual_end_time = libribasim.get_end_time()
Expand All @@ -39,7 +39,7 @@ def test_get_end_time(libribasim, basic, tmp_path):


def test_update(libribasim, basic, tmp_path):
basic.write(tmp_path)
basic.write(tmp_path / "ribasim.toml")
config_file = str(tmp_path / "ribasim.toml")
libribasim.initialize(config_file)
libribasim.update()
Expand All @@ -48,7 +48,7 @@ def test_update(libribasim, basic, tmp_path):


def test_update_until(libribasim, basic, tmp_path):
basic.write(tmp_path)
basic.write(tmp_path / "ribasim.toml")
config_file = str(tmp_path / "ribasim.toml")
libribasim.initialize(config_file)
expected_time = 60.0
Expand All @@ -58,15 +58,15 @@ def test_update_until(libribasim, basic, tmp_path):


def test_get_var_type(libribasim, basic, tmp_path):
basic.write(tmp_path)
basic.write(tmp_path / "ribasim.toml")
config_file = str(tmp_path / "ribasim.toml")
libribasim.initialize(config_file)
var_type = libribasim.get_var_type("volume")
assert var_type == "double"


def test_get_var_rank(libribasim, basic, tmp_path):
basic.write(tmp_path)
basic.write(tmp_path / "ribasim.toml")
config_file = str(tmp_path / "ribasim.toml")
libribasim.initialize(config_file)
actual_rank = libribasim.get_var_rank("volume")
Expand All @@ -75,7 +75,7 @@ def test_get_var_rank(libribasim, basic, tmp_path):


def test_get_var_shape(libribasim, basic, tmp_path):
basic.write(tmp_path)
basic.write(tmp_path / "ribasim.toml")
config_file = str(tmp_path / "ribasim.toml")
libribasim.initialize(config_file)
actual_shape = libribasim.get_var_shape("volume")
Expand All @@ -84,7 +84,7 @@ def test_get_var_shape(libribasim, basic, tmp_path):


def test_get_value_ptr(libribasim, basic, tmp_path):
basic.write(tmp_path)
basic.write(tmp_path / "ribasim.toml")
config_file = str(tmp_path / "ribasim.toml")
libribasim.initialize(config_file)
actual_volume = libribasim.get_value_ptr("volume")
Expand All @@ -97,7 +97,7 @@ def test_err_unknown_var(libribasim, basic, tmp_path):
Unknown or invalid variable address should trigger Python Exception,
print the kernel error, and not crash the library
"""
basic.write(tmp_path)
basic.write(tmp_path / "ribasim.toml")
config_file = str(tmp_path / "ribasim.toml")
libribasim.initialize(config_file)

Expand Down
Loading