marrink-lab / bentopy

Packs stuff in boxes
4 stars 0 forks source link

Behaviour when no compartments are defined #26

Open jan-stevens opened 6 months ago

jan-stevens commented 6 months ago

For basic packing functionality, it would be handy if we could also pack systems without defining a compartment. Currently this throws an error.

I think the ideal behavior should be:

This would make it simpler to pack cubic cytoplasms without having to define a "main" compartment. (this seems a bit convoluted?)

ma3ke commented 6 months ago

I agree that it would be nice to be able to omit the compartment definition.

That does leave the question of defining what compartment a segment belongs to in the "segments" section. Should that default just be called “main” implicitly? Or should an empty/unset compartments field for a segment imply the unset main compartment.

Setting a default compartment is a trivial change but it will only improve the interface if the segment definitions still make sense.

What do you think would be the neatest way of doing that?

jan-stevens commented 6 months ago

I think the easiest way to do this is to always create an implicit 'main' compartment at the start of our routine, which is just the box dimensions. Every segment is then, by default, part of this 'main' compartment.

I'm not sure if this will significantly impact performance. I imagine that having this extra compartment/mask will be noticeable for very large systems. Is this a problem?

ma3ke commented 6 months ago

The performance matters are not the concern here, in my initial estimate :)

What does slightly worry me is the confusion that comes from a default compartment. But if we do it like you propose, I think, such that

In case the compartment is specified, the implicit default for the segment compartment ids are not valid, and throws a good error about it.

That is all to prevent the following scenario: a specific (set of) compartment(s) is specified yet no segment has any compartment ids. That empty result would be very confusing otherwise.

ma3ke commented 6 months ago

If you do not set compartments in the space section, but do provide the compartments field in any of the segments in the segments section, you get the following error:

            If no `compartments` are defined in the `space` section,
            compartment selections in segment definitions are not allowed.
            When no compartments are defined in the `space` section, a single
            default compartment is implied. To set each segment's compartment
            to the implied default compartment, remove the `compartments`
            field from all segment definitions.

Though a bit wordy, I do think it communicates what is going on.

With this error in place, I'm comfortable moving forward with implementing this. Almost done :)

ma3ke commented 6 months ago

@jan-stevens, please let me know if you find these changes to your liking and that they properly reflect your suggestion.

Especially the note in the README (4673fca) about these defaults.

Something I am not entirely certain about is the following: currently, when any compertment is defined in the space section, a segment can have a compartments field, or omit the field. When it is omitted, a segment is understood to be in the implied "main" compartment, regardless of whether any compartment is actually called "main" or not. If this is considered problematic, there are at least two possible ways to improve this:

If everything looks good we can merge the PR, I think.