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

Refactor Python code #580

Merged
merged 15 commits into from
Sep 14, 2023
Merged

Refactor Python code #580

merged 15 commits into from
Sep 14, 2023

Conversation

evetion
Copy link
Member

@evetion evetion commented Sep 7, 2023

Fixes #512

  • Add JSON Schema of Config and generate Python code
  • Autogenerate Pandera Schemas
  • Update docs to include the above
  • Refactor Python model into split Config & Root submodels

@evetion evetion marked this pull request as ready for review September 12, 2023 09:17
@evetion
Copy link
Member Author

evetion commented Sep 12, 2023

I started with splitting the Model into separate tables and config, but didn't get far without hitting the limits of the current design.

The problem is, there's no clear design, it's mix of different abstractions. Pandera DataFrames are high-level abstract things, but represent low-level tables (which can come from arrow or geopackage). There's a high-level abstraction of a nodetype in between, but it doesn't represent something low-level. The current Model is a mix of a representation of these nodetypes with a (partial) low-level config. Will make a separate issue about this, which needs a refinement.

Copy link
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

A few, first comments.
I also see that mypy is still unhappy with you.

Will documentation still be part of this PR?

core/src/config.jl Show resolved Hide resolved
core/src/config.jl Show resolved Hide resolved
@@ -60,7 +80,7 @@ end

os_line_separator() = Sys.iswindows() ? "\r\n" : "\n"

function gen_schema(T::DataType, prefix = prefix)
function gen_schema(T::DataType, prefix = prefix; pandera = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is pandera optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I hacked the remarks field into each type specifically for pandera. But the generation of the Config doesn't need it.

Frankly, this whole file is a hack, but it works. Doing it right requires a new package that actually implements the whole of JSONSchema.

@evetion
Copy link
Member Author

evetion commented Sep 12, 2023

I also see that mypy is still unhappy with you.

Yeah, not sure if I can fix that nicely, I dynamically add things to a module namespace. Will try though.

Will documentation still be part of this PR?

Yep, at least about the generation of the config.py files.

@evetion evetion mentioned this pull request Sep 12, 2023
core/src/config.jl Outdated Show resolved Hide resolved
python/ribasim/tests/test_model.py Show resolved Hide resolved
python/ribasim/ribasim/config.py Outdated Show resolved Hide resolved
python/ribasim/ribasim/config.py Outdated Show resolved Hide resolved
@visr
Copy link
Member

visr commented Sep 14, 2023

Is it possible to remove the timestamp from the generated python files?

It creates merge conflicts:

<<<<<<< HEAD
#   timestamp: 2023-09-05T14:00:34+00:00
||||||| 1ad1340
#   timestamp: 2023-08-21T14:05:19+00:00
=======
#   timestamp: 2023-09-04T11:45:02+00:00
>>>>>>> main

And with #594 I expect we will run it more often as part of tasks, even if it is not strictly needed. In that case all it would do is update the timestamp.

@visr visr merged commit a8d18bc into main Sep 14, 2023
@visr visr deleted the feat/python-refactor branch September 14, 2023 15:46
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.

Refactor Python Model
3 participants