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

Correct parameter for the subnet size in adaptive search #140

Merged
merged 3 commits into from
Jun 28, 2022

Conversation

snilsn
Copy link
Collaborator

@snilsn snilsn commented Jun 20, 2022

This pull request aims at solving issue #133.

If neither adaptive_step nor adaptive_stop are given, MAX_SUB_NET_SIZE is set, otherwise MAX_SUB_NET_SIZE_ADAPTIVE is changed in the linking_trackpy()-function.

@freemansw1 freemansw1 added the bug Code that is failing or producing the wrong result label Jun 21, 2022
@freemansw1 freemansw1 linked an issue Jun 21, 2022 that may be closed by this pull request
@freemansw1 freemansw1 added this to the Version 1.5 milestone Jun 21, 2022
@freemansw1
Copy link
Member

I've requested @JuliaKukulies and I as reviewers for this. I should be able to get to this next week.

Once this is reviewed, it may make sense to release it as part of a 1.3.2 bugfix release. I'm hesitant to try to get it into 1.3.1, but I could see this and #124 for example making it into a 1.3.2 release. For now, I've kept it on the 1.5 milestone, but may move it to a new 1.3.2 milestone.

Copy link
Member

@JuliaKukulies JuliaKukulies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @snilsn, very good and careful observation! No objection from my side for the code changes you made. The only thing I wonder is whether we can add a more clear description for the subnetwork_size parameter in the docstrings (I mean to make clear that it means different things dependent on whether you specify adaptive_step/adaptive_stop or not). As I understand it, you would want it to be rather small when you do adaptive search and rather large when you do not. So this is important information the user needs to know.

@freemansw1 freemansw1 modified the milestones: Version 1.5, Version 1.3.2 Jun 22, 2022
Copy link
Member

@freemansw1 freemansw1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with these changes. Good catch on finding the bug.

@freemansw1
Copy link
Member

With the approving reviews, @snilsn please confirm that you're ready to merge and we will get this in dev with the intent to release as part of v1.3.2.

@snilsn
Copy link
Collaborator Author

snilsn commented Jun 28, 2022

Yes, from my side everything is ready for merging.

The only thing I wonder is whether we can add a more clear description for the subnetwork_size parameter in the docstrings

I will adress this in an update of PR #138

@JuliaKukulies
Copy link
Member

Good, yes agreed that the documentation update is better for #138 ! Thanks!

@freemansw1 freemansw1 merged commit 90fb3ff into tobac-project:dev Jun 28, 2022
@snilsn snilsn deleted the adaptive branch July 6, 2022 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Code that is failing or producing the wrong result
Projects
None yet
Development

Successfully merging this pull request may close these issues.

subnet_size in linking_trackpy() does not work with adaptive search
3 participants