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

Optimize get_structure_mask for structures with many children #120

Merged
merged 1 commit into from
Jun 8, 2022

Conversation

sdiebolt
Copy link
Contributor

get_structure_mask can be optimized for structures with many children: see #119.

Thanks so much for creating and maintaining this awesome package! I'm not yet used to contributions on GitHub, feel free to let me know if I can improve anything.

@vigji
Copy link
Member

vigji commented May 21, 2022

@FedeClaudi @adamltyson tests are running fine locally on my machine, but there seems to be something funky in GitHub actions going on - would you agree to merge and fix tests in a separate PR?

@FedeClaudi
Copy link
Contributor

I'm happy with it

@adamltyson
Copy link
Member

TBH I'd rather we at least tried to fix the tests within the PR, unless it turns out to be a separate problem. Otherwise I fear we'll never get round to it.

@sdiebolt
Copy link
Contributor Author

sdiebolt commented May 23, 2022

It seems like the fail comes from the dependency resolver: it should not have anything to do with the changes from this PR.

You might also want to try running the tests again: as the dependencies didn't change since last commit, it might either be a one-time bug from pip, or a problem introduced by a new version of one of the dependencies (in which case you might want to try to be more strict in requirements.txt?).

If the dependency resolver still fails after a rerun, I can try to reproduce it on my machine in the following days!

@sdiebolt
Copy link
Contributor Author

Still there after a rerun... I just tried running pip install .[dev] on my machine with Python 3.8.12 (as defined in the GitHub Actions workflow) and there is indeed a lot of backtracking. I think this could be solved very simply by setting stricter version limits in requirements.txt!

Changing requirements.txt is best left to someone who is more aware of the exact use of each of those dependencies in BG-AtlasAPI, but do tell me if I can help. 🙂

@vigji
Copy link
Member

vigji commented May 23, 2022

Yes it's definitively not on you @sdiebolt to fix! I'll try to give it a shot in the next days

@adamltyson
Copy link
Member

Having looked at this again, sorry @sdiebolt, you're right, the tests failing is nothing at all to do with this PR, we should merge this now, and fix the test failure separately.

@sdiebolt
Copy link
Contributor Author

sdiebolt commented Jun 2, 2022

No problem, that is indeed a weird test failure!

@adamltyson
Copy link
Member

@vigji are we good to merge? I haven't tested this myself yet.

@vigji
Copy link
Member

vigji commented Jun 8, 2022

yes, all tests are running locally! Although we'll break tests for main I guess

@adamltyson adamltyson merged commit 79db485 into brainglobe:master Jun 8, 2022
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.

4 participants