add check for missing timezone.

[bendichter]

Also add best practice docs and tests for check

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.91%. Comparing base (2e42e34) to head (7631456).
Report is 3 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #458      +/-   ##
==========================================
+ Coverage   86.78%   86.91%   +0.12%     
==========================================
  Files          23       23              
  Lines        1241     1253      +12     
==========================================
+ Hits         1077     1089      +12     
  Misses        164      164              

Flag	Coverage Δ
unittests	86.91% <100.00%> (+0.12%)	⬆️

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

Files	Coverage Δ
src/nwbinspector/checks/nwbfile_metadata.py	99.20% <100.00%> (+0.03%)	⬆️

[bendichter]

It looks like this is (edit: were) failing because there is a workflow that explicitly tests older versions of pynwb and this fails in that case since the feature is only supported in 2.7.0+. What should we do here? Here are some options:

1. Add a skipif for the offending tests. I have implemented this here. This works for getting past the error, but the check will malfunction. That is not so bad in this particular case, since the error is erroneously silent, which is fine here since the user would not be able to fix that issue with their version of PyNWB anyway.
2. Since NWB Inspector is about best practices, it may make sense to have strict requirements about only using the latest PyNWB. This makes sense from a best practices perspective, but may not be practical for everyone. For example, the Allen Institute has in the past found it difficult to easily migrate to new environments.
3. We could introduce a skipif arg to the @register_check decorator, which would allow us to explicitly label checks as pertaining to certain conditions or configurations. This would solve the issue, but would fail to notify users that they could improve best practices if they upgrade pynwb. I suppose we could get around this downside by adding a check specifically for pynwb version, ensuring it is the latest release.

[bendichter]

Another question about this is how this would be integrated with NWB GUIDE. Does the GUIDE have a way to enter timezones yet? If not, we are setting users up for a best practice warning they are not able to address.

[CodyCBakerPhD]

It looks like this is (edit: were) failing because there is a workflow that explicitly tests older versions of pynwb and this fails in that case since the feature is only supported in 2.7.0+. What should we do here?

Yeah this has been a long-time question about how the Inspector should interact with PyNWB versions in general (in addition to cached schema within each file??): #263

Posted your ideas on that thread

Skipping per test is fine for now and has been the way forward for the time being

[CodyCBakerPhD]

Another question about this is how this would be integrated with NWB GUIDE. Does the GUIDE have a way to enter timezones yet? If not, we are setting users up for a best practice warning they are not able to address.

It looks like it still automatically sets based on computer clock

I think we were partially waiting on finding a better timestamp widget, but @garrettmflynn for the time being can you add a simple dropdown selector for $\pm \ X$ ($X \in {0, ... 12}$) to at least allow it? We can polish UX over time

[bendichter]

Codespell:

Error: ./tests/test_register_check.py:17: assertIn ==> asserting, assert in, assertion

Is codespell checking the spelling of code? How can we get it to skip this?

[CodyCBakerPhD]

Is codespell checking the spelling of code? How can we get it to skip this?

Hah, looks like an issue for very recent versions of codespell - already some others have noticed: codespell-project/codespell#3430

Doing a local ignore now

[CodyCBakerPhD]

Also discovered that we added this to pre-commit at some point: https://github.com/NeurodataWithoutBorders/nwbinspector/pull/458/files#diff-63a9c44a44acf85fea213a857769990937107cf072831e1a26808cfde9d096b9L13

but didn't remove the GitHub action

[CodyCBakerPhD]

More pressing issue is some rather strange datetime comparison error that for whatever reason is fine on Windows but not elsewhere: https://github.com/NeurodataWithoutBorders/nwbinspector/actions/runs/9214133616/job/25349652811?pr=458#step:12:232

[bendichter]

@CodyCBakerPhD this is probably because datetime is a subclass of date. I personally find this counter-intuitive. Maybe it's not true on Windows?

import datetime
isinstance(datetime.datetime.now(), datetime.date)

True

[CodyCBakerPhD]

It's true on Windows. Very odd