Add mask subcommand - Githubissues

marrink-lab / bentopy

Packs stuff in boxes

4 stars 0 forks source link

Add mask subcommand #20

Closed ma3ke closed 3 months ago

ma3ke commented 3 months ago

This is a tracking pull request for the new mask subcommand.

[x] Test it for many inputs.
[x] Comb through the TODOs.
[x] Get the --no-interactive flag system up.
- This allows a user to say: "Please don't prompt me during the runtime of the command. If I didn't specify something consider that an error, not an invitation to wait for my input.

ma3ke commented 3 months ago

Tasks left:

[x] Display a percentage of placeable space with the occupied/available voxel summary.

ma3ke commented 3 months ago

In order of importance.

[x] --autofill
[x] Reference the mdvcontainment GitHub page.
[x] Improve wording for skippable inputs.

ma3ke commented 3 months ago

@BartBruininks, @jan-stevens

I am pretty happy with where the mask subcommand stands right now. Perhaps it is time to just merge it into main. Below, I will list the two things we can still add, but I do not see them as blockers by any means. I can make these points into enhancement issues, actually.

Let me know if you agree with merging.

[ ] [yes]/no/path where yes uses the suggested default path.
- Matter of taste, somewhat, but could be nice.
[ ] Represent the containment graph like a directory structure akin to tree.
- This would be an excellent feature to represent the containment graph in a format that can be understood from the terminal. Perhaps this is a function that can be hosted by the mdvcontainment package, such that we can call it to get a string we print to the terminal.

jan-stevens commented 3 months ago

I tried out the latest version of the mask subcommand, and I think the functionality is awesome!

One nitpick, I would suppress the MDAnalysis warnings with:

import warnings
warnings.filterwarnings("ignore")

But for the rest LGTM, I'm fine with merging into main.

ma3ke commented 3 months ago

One nitpick, I would suppress the MDAnalysis warnings [...]

I believe that this was already resolved by 89d1d83803aa42c6c830057e2dcd8b486a2ddef5 but perhaps something is up that makes the problem invisible on my setup while it does show up on your system. For now, I am pretty confident that it should work, and if it doesn't well just patch that up over on main.