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

Warn if a non-version key is duplicated in epi_df or epi_archive #154

Closed
nmdefries opened this issue Jul 21, 2022 · 5 comments
Closed

Warn if a non-version key is duplicated in epi_df or epi_archive #154

nmdefries opened this issue Jul 21, 2022 · 5 comments
Labels
enhancement New feature or request P2 low priority REPL Improved print, errors, etc.

Comments

@nmdefries
Copy link
Contributor

Currently, we do not check for distinct key values. This has some potential uses but also introduces the opportunity to form unexpected or invalid archives. Continue allowing this behavior, but warn the user if duplicate keys exist when creating or adding to an archive or an epi_df.

@nmdefries nmdefries added enhancement New feature or request P2 low priority REPL Improved print, errors, etc. labels Jul 21, 2022
@brookslogan
Copy link
Contributor

#89 discusses this as well. This issue is definitely the first step to do. However, we may later decide to either (a) turn it into an error, or (b) make things actually work when there is a warning. Actually, since this breaks the key as_of function, and thus also epix_slide, as well as the current epix_merge defaults (+ makes the user have to do post-processing to make non-defaults work) and stops the overhauled epix_merge in #101 from running, I think maybe we do want to just go ahead and make it an error. Probably not helpful to construct an epi_archive with this condition if most things don't work and something works but requires manual hacking to fix up issues.

@brookslogan
Copy link
Contributor

If we do keep it a warning, we need it to be a dire warning like "Current epi_archive methods were built assuming the key was a unique key; e.g., epix_as_of and epix_slide will give incorrect results, and other methods may also malfunction."

@brookslogan
Copy link
Contributor

epi_archive side of this is handled in lcb/grouped_epi_archive. Just made it a hard error since key methods like as_of malfunction otherwise. Decision might differ for epi_df.

@brookslogan
Copy link
Contributor

Being addressed in #519

@nmdefries
Copy link
Contributor Author

Addressed in #519

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P2 low priority REPL Improved print, errors, etc.
Projects
None yet
Development

No branches or pull requests

2 participants