Add python package for 2DTM postprocessing - #548
Conversation
|
Hi @kekexinz glad to see this making its way in. A few things: 1 - It looks like your setup script does not invoke or create a virtual environment. While I can see why you may want the user to be able to determine this, I feel pretty strongly that the default should be to create a venv at setup or allow the user to ignore that default and use whatever is active. 2 - I also don't think this needs to live on a separate branch in order to allow agile development.Let's instead create a structure under the main working folder where we have a "python" directory and then below that a 2DTM_post_processing directory. (I imagine some amount of what you have in here will be more general for other interfaces with cisTEM using python, so code may eventually migrate out into a more general python "core" or "utils" 3 - we can modify both the PR to indicate, "only changes python" or "only changes python/subproject, which will lower the bar on PR passing. 4 - we can also, in principle get around requiring review by adding some basic CI that checks that some basic requirements are met, and also checks that only python/subproject is changed, if so automerge. (Don't require external review.) I don't think this is necessary though if it is clear the bar is lower and we can have a "semi" automated review. What do you think? |
|
Hi @kekexinz I've update the most recent devcontainer to have a base python vs of 3.10 in Ubuntu 22 and it runs a dedicated venv as well (which is probably redundant in the container, but this way it mirrors what you can run outside the container.) As we had spoken about via email, I'll pull this PR locally and make a few changes to the file organization and then merge it into the new "main" branch here. One of the major design shifts coming up is to switch to have containerized release in addition to dev, and that will make it much easier to have python be a "first class" part of the project, rather than living in some branch. |
|
Awesome, thanks for the update @bHimes ! Once you’ve pushed the reorganized version, I’ll rebase my branch and update any dependent scripts. |
Description
This PR adds a new folder containing a standalone Python library.
⚠️ Note for maintainers: I am proposing that these changes be merged into a new branch (e.g.
The C++ codebase is untouched — this is an additive change only.
The goal is to add the Python tool while keeping it isolated from the core C++ code.
No existing functionality or build system is affected.
2DTM_postprocess_tool) rather than directly intomaster, so that the Python work can evolve separately from the main C++ code.Fixes # (issue)
N/A — this is a new addition, not a fix.
I have rebased my feature branch to be current with the master branch using to minimize conflicts and headaches
Which compilers were tested
N/A — no C++ changes. This PR only adds Python code.
These changes are isolated to the
How has the functionality been tested?
The Python package builds and installs with: