Skip to content

Final Polishing core.py #396

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

Open
wants to merge 57 commits into
base: develop
Choose a base branch
from
Open

Final Polishing core.py #396

wants to merge 57 commits into from

Conversation

giovannivolpe
Copy link
Member

No description provided.

@giovannivolpe
Copy link
Member Author

This will also address issue #371

@giovannivolpe giovannivolpe linked an issue Aug 1, 2025 that may be closed by this pull request
def test_DeepTrackDataObject(self):
dataobj = core.DeepTrackDataObject()

# Test storing and validating data.
# Test default inititialization
Copy link
Collaborator

Choose a reason for hiding this comment

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

initialization

Copy link
Collaborator

@mirjagranfors mirjagranfors left a comment

Choose a reason for hiding this comment

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

Looks very good! I left a few comments on things that could be made even clearer

>>> child.is_valid((0,))
True
>>> grandchild.is_valid((0,))
False

Setting a value and automatic invalidation:
Copy link
Collaborator

Choose a reason for hiding this comment

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

When reading this comment, I expect to see something about the automatic invalidation in the example. But it is not obvious to me which data is automatically invalidated


# Invalidate parent and check child validity.
parent.invalidate()
self.assertFalse(parent.is_valid())
self.assertFalse(child.is_valid())
self.assertFalse(grandchild.is_valid())

# Validate parent and ensure child is invalid until recomputation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that "# Validate child. Parent and grandchild remain invalid until recompute." would be clearer here

child.validate()
self.assertFalse(parent.is_valid())
self.assertTrue(child.is_valid())
self.assertFalse(grandchild.is_valid())

# Recompute child and check its validity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here I would prefer "# Recompute grandchild and check its validity."

" ┌────────────┐ ┌────────────┐\n",
" │ process1_B │ │ process2_B │\n",
" └────────────┘ └────────────┘\n",
" ↓ ↘ ↙ ↓\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that by removing 2 of the arrows in this row the figure would become clearer

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.

Add new() to Feature
3 participants