Conversation
…ewis-Seequent/evo-python-sdk into add-kriging-compute-preview # Conflicts: # mkdocs/site/packages/evo-python-sdk.html
packages/evo-compute/src/evo/compute/tasks/common/source_target.py
Outdated
Show resolved
Hide resolved
packages/evo-compute/src/evo/compute/tasks/common/source_target.py
Outdated
Show resolved
Hide resolved
…y it did not work properly during prototyping.
…o add-kriging-compute-preview
…o add-kriging-compute-preview
… reducing file sizes a bit.
PaulCaygill-Seequent
left a comment
PaulCaygill-Seequent
left a comment
There was a problem hiding this comment.
From my perspective, the stuff to do with running a remote Kriging task seems fine. You've not (that I can see) written anything around that area that would create massive problems, just added wrappers around existing logic and clients etc.
I commented on the thing or two that I could see that felt odd, but this could be feedback driven changes for all I know
…llow easier extension.
…nning notebook environment anyway.
grantroch
left a comment
grantroch
left a comment
There was a problem hiding this comment.
Should the html live in the repository?
From what I can see this looks good from the index pages and Kriging task. I like the simple examples in the index pages. It would have been nice to separate the documentation changes from the functional changes around the Kriging task.
mkdocs/docs/packages/evo-blockmodels/typed-objects/regular-block-model/RegularBlockModel.md
Show resolved
Hide resolved
mkdocs/gen_api_docs.py
Outdated
| # --------------------------------------------------------------------------- | ||
| # Config: packages whose typed source files are auto-discovered. | ||
| # Every non-_-prefixed .py with an __all__ produces a subfolder group under | ||
| # typed-objects/ named after the file stem (underscores → dashes). | ||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
I don't think these comment headers are necessary. If we're relying on these comments, surely the classes/functions could be organised in a nicer way with better/clearer naming? Perhaps this could also be split up between multiple files, as it's getting quite big now.
There was a problem hiding this comment.
Under 250 lines I think is a good standard for the code, and if we would like to enforce even harsher limits than that py-linting is probably a better place to start and review our standards to avoid long files.
There was a problem hiding this comment.
It actually looks a lot nicer without the comments everywhere, I think I'm mostly happy now
| show_root_heading: true | ||
| separate_signature: true | ||
| show_signature_annotations: true | ||
| show_labels: false |
There was a problem hiding this comment.
What is the reasoning and implications for this change?
|
@GriffinBaxterSeequent removed CHANGELOG changes and done a final tweak on autogen file. Not planning to tweak it further it is readable enough and compact enough, breaking it into separate files is not a good idea in my opinion. Separated references to typed packages into .txt (though not expecting it to change much in near future). |
RyanMillerSeequent
left a comment
RyanMillerSeequent
left a comment
There was a problem hiding this comment.
LGTM, only looked at files owned by objects-maintainers
6 participants
Adding kriging compute that can be ran with preview flag.
Extending a lot of documentation and updating the changlog, as now the package should be in a good state to release without further outstanding changes.