Added optional metadata_file_path to get_metadata

[pauladkisson]

Fixes #541

[codecov]

Codecov Report

❌ Patch coverage is 98.06452% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.06%. Comparing base (304691d) to head (07c99dc).

Files with missing lines	Patch %	Lines
...aces/ophys/scanimage/scanimageimaginginterfaces.py	83.33%	2 Missing ⚠️
...es/ophys/tdt_fp/tdtfiberphotometrydatainterface.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1448      +/-   ##
==========================================
+ Coverage   87.04%   87.06%   +0.01%     
==========================================
  Files         145      145              
  Lines        9681     9692      +11     
==========================================
+ Hits         8427     8438      +11     
  Misses       1254     1254              

Flag	Coverage Δ
unittests	87.06% <98.06%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/neuroconv/basedatainterface.py	80.00% <100.00%> (+0.65%)	⬆️
...nv/datainterfaces/behavior/audio/audiointerface.py	90.52% <100.00%> (ø)
...ces/behavior/deeplabcut/deeplabcutdatainterface.py	90.78% <100.00%> (ø)
...nterfaces/behavior/fictrac/fictracdatainterface.py	92.25% <100.00%> (ø)
...s/behavior/lightningpose/lightningposeconverter.py	95.16% <100.00%> (ø)
...havior/lightningpose/lightningposedatainterface.py	92.74% <100.00%> (ø)
...atainterfaces/behavior/medpc/medpcdatainterface.py	95.45% <100.00%> (ø)
...faces/behavior/miniscope/miniscopedatainterface.py	100.00% <100.00%> (ø)
...aces/behavior/neuralynx/neuralynx_nvt_interface.py	98.36% <100.00%> (ø)
...nterfaces/behavior/video/externalvideointerface.py	96.55% <100.00%> (ø)
... and 40 more

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[h-mayorquin]

I am against this. I think this makes our codebase more complicated (see the size of the PR) for not a lot of gain (saving some lines). I also think that that the behavior of implicitly doing a dict deep update when loading the data is a non-obvious side effect that will cause bad surprises to some of our users.

Explicit is better than implicit.

[h-mayorquin]

If we really want to move forward with this feature, perhaps we should refine our method naming to be more explicit and avoid mixing behaviors. Some of the current behaviors include:

• get_source_metadata: retrieves the metadata that is available in the source
• get_default_metadata: retrieves the default metadata needed to write the interface to NWB (in practice, aside from session_start_time, we don’t really have other strict requirements)

Currently, get_metadata combines the two behaviors above.

• loading_metadata_from_file: this is essentially what load_dict_from_file does, although it’s not defined as an interface method.

The proposed method:
name_that is_to_verbose_to_be_defined: retrieves metadata from both the source and defaults, then loads the file from YAML and deep-updates the dictionary so the user can modify it further.

This seems like too many responsibilities for a single method. Maybe we could consider designing a better framework that supports composition of the above behavior and then offer this desired functionality as a wrapper function. That way, the method name can be more explicit about what it actually does and the internal behavior more explicitly documented by functions.

[pauladkisson]

I'm happy with splitting up metadata into source and default (although, I think that would take significantly more work to untangle the two different types from our current get_metadata methods). But I really don't see what's wrong with packaging that up into a high-level get_metadata function. For example,

def get_metadata(self, metadata_file_path: FilePath | None):
    default_metadata = self.get_default_metadata()
    source_metadata = self.get_source_metadata()
    metadata = dict_deep_update(default_metadata, source_metadata)
    if metadata_file_path is not None:
        file_metadata = load_dict_from_file(metadata_file_path)
        metadata = dict_deep_update(metadata, file_metadata)
    return metadata

[h-mayorquin]

But I really don't see what's wrong with packaging that up into a high-level get_metadata function. For example,

Packing it up on a high-level function with lots of bells and whistle is OK. But I think there should be a function that performs the basic function that has a clear name, no side-effect and does not have a lot of complexity. For example, we have both add_to_nwbfile (simple and and clear action from the name) and run_conversion (less simple and has a lot of options).

Now perfect should not be the enemy of done when adding features but I don't think this features is ... that important. As I mentioned above, it is only saving the user from writing a couple of lines. But overall, fair, maybe get_metadata should go the way of run_conversion and be the high level method. Also:

(although, I think that would take significantly more work to untangle the two different types from our current get_metadata methods).

I don't think this should be on you.