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

Clean up extra files created by running docs + tests #226

Open
thomas-rocke opened this issue Jan 16, 2024 · 7 comments
Open

Clean up extra files created by running docs + tests #226

thomas-rocke opened this issue Jan 16, 2024 · 7 comments

Comments

@thomas-rocke
Copy link
Contributor

test_dislocation.py produces a dd_test.png, and some docs notebooks save a few files when docs are built - all of which appear on git status. We should either try to clean them up after docs are built, or add them to the gitignore.

@jameskermode @pgrigorev do we think the test_differential_displacement still has much value? It only serves to generate the dd_test.png image, and it's using the BCC-specific make_screw_cyl method to do so, rather than the CubicCrystalDislocation solution.

As for the files created by the docs, a good solution might be to have a teardown.py file to do the cleanup, which could be called by make html etc after Sphinx docs have been built? I think it would also be better practice to save such files to a tmp/ directory which could be gitignored, but this is less important.

@jameskermode
Copy link
Member

Agree we should clean those files up in a tearDown(), and/or use temp files.

@jameskermode
Copy link
Member

jameskermode commented Jan 16, 2024

If tearDown() was added as a method to the corresponding test class it would run automatically

@thomas-rocke
Copy link
Contributor Author

Only issue is that the teardown() would basically undermine the whole point of the "test" - it doesn't actually assert/check anything, and I think it was intended as a visual check that the disloc was doing the right thing. teardown() would delete the file before a visual check could be done, so we may as well just delete the test if we don't want dd_test.png to live any longer than pytest is running.

@jameskermode
Copy link
Member

It still checks the codepath by which the figure is produced runs without error, increasing test coverage, but I don't have a strong feeling about this so will defer to @pgrigorev

@pgrigorev
Copy link
Contributor

Hello, thank you very much for cleaning up the module and the test. I think make_screw_cyl is still useful to screw make kink configurations. I tried to get rid of this function, but never managed to finish it. I would suggest to keep it for the moment. Maybe add a warning that it is deprecated and people use the methods buit in the dislocation class. For the test of differential displacement you are correct that it was just for visual control and also to have something that calls atomman function to see if it is broken (this happened before quite often). I plan to implement the vitek maps without the dependency on atomman. So we will get rid of this at some point. If you want to remove it now it is fine for me as well.

@thomas-rocke
Copy link
Contributor Author

Thanks @pgrigorev for the input. I have the same feeling about the BCC-specific code - I know the BCC quadrupoles are better with the earlier functions than with my more general CubicCrystalDislocationQuadrupole solution (see #206) so they should definitely be kept. It may be nice to have a chat at some point to go throught these functions and see which ones are better than their CubicCrystalDislocation counterparts with the aim of deprecating some.

In terms of dd_test.png, maybe we just comment out the part that saves the figure, so that the test just validates the code path runs without errors? Happy to work on these things, as it doesn't sound like a massive time sink.

@pgrigorev
Copy link
Contributor

Hi @thomas-rocke. For sure would love to have a chat about that. Also about Vitek maps since I wanted to do something similar to what you did with stacking faults: visualisation function as class method that has all the system specific parameters under the hood. I will be very busy in coming couple of weeks, but after that for sure will have time for that.

I think commenting out the part that saves the png is a good solution. I am happy with it. Thank you!

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

No branches or pull requests

3 participants