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

allow datetimes to be just dates #542

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

allow datetimes to be just dates #542

wants to merge 6 commits into from

Conversation

bendichter
Copy link
Contributor

Summary of changes

  • ...

Checklist

For all schema changes:

  • Add release notes for the PR to docs/format/source/format_release_notes.rst.

If this is the first schema change after a schema release (i.e., the version string in core/nwb.namespace.yaml does not
end in "-alpha"), then:

  • Update the version string in core/nwb.namespace.yaml and core/nwb.file.yaml to the next major/minor/patch
    version with the suffix "-alpha". For example, if the current version is 2.4.0 and this is a minor change, then the
    new version string should be "2.5.0-alpha".
  • Update the value of the version variable in docs/format/source/conf.py to the next version without the
    suffix "-alpha", e.g., "2.5.0".
  • Update the value of the release variable in docs/format/source/conf.py to the next version with the suffix
    "-alpha", e.g., "2.5.0-alpha".
  • Add a new section in the release notes docs/format/source/format_release_notes.rst for the new version
    with the date "Upcoming" in parentheses.

@rly
Copy link
Contributor

rly commented Feb 6, 2024

I get the need for having just dates (e.g., date of birth). What is the use case for a time without a timezone? I get that the timezone might be unknown and it is a pain to enter, but a time without a timezone is not that informative, IMO?

@t-b
Copy link
Contributor

t-b commented Feb 6, 2024

I get that the timezone might be unknown and it is a pain to enter, but a time without a timezone is not that informative, IMO?

I fully agree. A time without a timezone is not a proper time.

@bendichter
Copy link
Contributor Author

bendichter commented Feb 6, 2024

The day and time of day is important, as it pertains to experiment records as well as the functioning biology of the animal (feeding schedule, sleep cycle, hormone cycle, etc.). I recognize that leaving out a timezone would make the time impossible to unambiguously resolve against a global clock. I just think the global time of an experiment is of very little actual importance in the analysis of neural data. I have never needed this value, I don't think I have ever come across an analysis that needed this, and I can't think of any actual applications that would have required this (feel free to offer applications if I am missing something here).

Requiring this field causes a lot of friction:

  1. This requires manual correction as nearly no acquisition systems provide this value, making it difficult to automate conversion from most if not all acquisition systems. When working with DataJoint, this was again a friction point in building automated export tools from their element databases to NWB. Removing this requirement would allow us to remove ~60 lines of code in our NeuroConv tutorials.
  2. I would predict that many users do not set this value, allowing PyNWB to select the default value, which is the local timezone, and may be incorrect.

If this is causing friction, is probably often incorrect, and is providing dubious value, I think it should be optional.

I could imagine some hypothetical scenarios where a timezone might be useful, for example if you were dealing an organism with a short lifespan like C. elegans, and the organism was for some reason being transported across timezones. In cases like this I would say it should be best practice to include the time zone. We can easily codify that in the NWB Best Practices.

@t-b
Copy link
Contributor

t-b commented Feb 7, 2024

If someones downloads a dataset from dandi, the timezone of the downloader is mostly different from the timezone of the person writing the NWB file. So there you have already lost information without timezone.

If pynwb sets a default timezone and people ignore the warning, this is definitly a problem. But I don't see a reason in this case to drop the timezone.

Edit: It is not about if you need the timezone in analysis. The timezone is part of the timestamp, without it a timestamp is much less accurate.

@rly
Copy link
Contributor

rly commented Feb 8, 2024

While a timestamp without a timezone is not a globally accurate time, from my experience, it would be extremely rare, if it ever happens, that the timezone that a neurophysiological dataset was collected in matters. If the detail is not useful and requiring it causes problems, why require it?

I think if the datetime is written without timezone, the API should treat the datetime as lacking timezone (timezone naive) and the user should interpret the datetime as relative to the experimenter's timezone, whatever it was. If a timezone is provided, it should still be read and used to create a timezone-aware datetime. The API should not provide a default timezone. Writing a timezone should still be allowed - just optional.

@t-b
Copy link
Contributor

t-b commented Feb 8, 2024

I don't agree but it is not my project.

@bendichter
Copy link
Contributor Author

@t-b practically speaking, requiring a timezone makes the timestamp more precise but does not make the timestamp more accurate because the the timezone is often wrong.

@bendichter
Copy link
Contributor Author

getting rid of timezones may be causing an issue on the DANDI side dandi/helpdesk#140 (reply in thread)

let's put this on hold until we sort this out

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.

3 participants