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

CoinSorter v2 advanced #410

Closed
wants to merge 0 commits into from
Closed

Conversation

kochebina
Copy link
Contributor

ref data for test072 is here:
https://doi.org/10.6084/m9.figshare.25992610.v1

@kochebina kochebina force-pushed the CoinSorter_v2 branch 2 times, most recently from f87ee87 to 4b9795a Compare September 16, 2024 15:08
@kochebina kochebina mentioned this pull request Sep 18, 2024
@kochebina
Copy link
Contributor Author

@tbaudier, could you, please, update the tests ?
Otherwise the PR will never pass the checks.

Or should I do the tests update in a different PR ?

@tbaudier tbaudier force-pushed the CoinSorter_v2 branch 4 times, most recently from aed5d25 to 9aadadb Compare October 9, 2024 09:09
@tbaudier tbaudier force-pushed the CoinSorter_v2 branch 3 times, most recently from 40f4602 to a60fef7 Compare October 16, 2024 14:13
@nkrah
Copy link
Collaborator

nkrah commented Nov 4, 2024

What's the status on this? The tests provided by this PR do not seem to pass. Is there an issue to be solved?

@kochebina
Copy link
Contributor Author

What's the status on this? The tests provided by this PR do not seem to pass. Is there an issue to be solved?

Hi Nils,

There are several things:

  1. New tests should be used and they are also in this PR (maybe I should use a new PR to add the new tests before?)
  2. The data was not enough specific --> Corrected
  3. Several additional bugs are found --> I am on it
    So, the work is in progress.

Cheers,
Olga

@nkrah
Copy link
Collaborator

nkrah commented Nov 5, 2024

Great.
No need for a new PR. Just push commits into this one. That might be new tests, or modified tests. You can also remove obsolete tests (the removal is tracked as a commit by github).
The data is handled via a separate repository, as you know. Please contact Thomas if you need help on that.

Please post here if you want anyone to review the code or if you think the PR is ready to be considered for merging.

@nkrah
Copy link
Collaborator

nkrah commented Nov 18, 2024

I am a bit confused: Is this PR linked to PR #557 ?
Is this still relevant or only #557 ?

@kochebina
Copy link
Contributor Author

@nkrah
Hi Nils, No.
As I wrote in my email last week: the release will be with the simplified coincidence sorter, thus, with corresponding documentation

@nkrah
Copy link
Collaborator

nkrah commented Nov 19, 2024

OK, so both PRs, #557 and this one (#410) are still WIP?
Sorry, but there is plenty of movement going on in the repo and it is better to be sure that we don't miss/mix up things.

It would be best to post PR related things here in the PR-related chat so communication remains linked together.

@kochebina
Copy link
Contributor Author

@tbaudier @nkrah @dsarrut

This PR is ready to be merged.
The new test data is here:
https://doi.org/10.6084/m9.figshare.28063244.v1

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.

2 participants