Skip to content
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

Mixed improvements #79

Merged
merged 15 commits into from
Jul 11, 2024
Merged

Mixed improvements #79

merged 15 commits into from
Jul 11, 2024

Conversation

kretep
Copy link
Member

@kretep kretep commented Jun 25, 2024

  • Fix typos, documentation
  • Validation for date format
  • Validate on write
  • Update workflows

@@ -48,11 +50,11 @@ class TSDFMetadata:

file_dir_path: str
""" A reference to the directory path, so we don't need it again when reading associated binary files. """
metadata_file_name: str
metadata_file_name: str #TODO: do we need this?? / is it used?
Copy link
Member

Choose a reason for hiding this comment

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

This is only used when we want to override the existing metadata file by updating the corresponding tsdf object, but that might be unnecessary.

Copy link
Member Author

@kretep kretep Jul 11, 2024

Choose a reason for hiding this comment

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

So when saving existing metadata at a new location, it is not needed to update this, right? Because that's how I see it being used often.

Copy link
Member

Choose a reason for hiding this comment

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

As far as I know, when saving to a new location the location is expected to be explicitly specified when writing to the disc (this is more of an implicit location tag)

:param metadata: dictionary containing the metadata.

:return: TSDFMetadata object.
:param file_dir: path to the directory where the file will be saved.
Copy link
Member

Choose a reason for hiding this comment

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

We could at one point convert all the documentation to NumPy style (we started it for the dbpd), just not to forget.

Copy link
Member

@vedran-kasalica vedran-kasalica left a comment

Choose a reason for hiding this comment

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

The PR looks good. The validation on the formatting was overdue, and the improved testing is quite useful!

@vedran-kasalica vedran-kasalica merged commit d84b59a into main Jul 11, 2024
1 check passed
@vedran-kasalica vedran-kasalica deleted the mixed-improvements branch July 11, 2024 15:27
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.

None yet

2 participants