Open rudrakshnalbalwar opened 1 month ago
*beep* *bop* Hi human, I ran ruff on the latest commit (8b00774ed53b81498e21f40e200c3207d947d10d). Here are the outputs produced. Results can also be downloaded as artifacts here. Summarised output:
Complete output(might be large):
*beep* *bop* Hi human, I ran benchmarks as you asked comparing master (3b6084f275342c30b5be913a11a76690cc56e1ff) and the latest commit (8b00774ed53b81498e21f40e200c3207d947d10d). Here are the logs produced by ASV. Results can also be downloaded as artifacts here.
Significantly changed benchmarks:
All benchmarks:
If you want to see the graph of the results, you can check it here
Thanks for the contribution! This is a good change, but I think you need to double check where this function is used in the codebase, and change any imports or function calls to point to the right location. I believe that this function is used in the LIV and SDEC plot classes. https://github.com/search?q=repo%3Atardis-sn%2Ftardis%20_parse_species_list&type=code
@jvshields I made the changes as you mentioned , I’ve removed the underscore from the parse_species_list function name as requested. However, I did not change the name of the test function test_parse_species_list, since it already follows the convention for testing this function. And I noticed that there were no explicit imports for the _parse_species_list function previously. Since the function is used within the same class/module, an import doesn't seem necessary in this case. Therefore, I haven't added an import for parse_species_list either.
Also one more thing do I have to make changes here too ? tardis/.pytest_cache/v/cache/nodeids @jvshields , @atharva-2001 , @andrewfullard
Please double check that the tests are passing.
I believe that the full refactor here should look like this.
First, move the parse_species_list() function to the util file. (You have done this and it looks good)
Next, remove the function definition from the sdec plot and liv plot classes. Import the more generic function from the util file.
Finally, wherever the function is used in those classes, make sure that it's working properly with the new imported function. The tests here will tell you if everything is hooked up correctly.
Thanks.
Please double check that the tests are passing.
I believe that the full refactor here should look like this.
First, move the parse_species_list() function to the util file. (You have done this and it looks good)
Next, remove the function definition from the sdec plot and liv plot classes. Import the more generic function from the util file.
Finally, wherever the function is used in those classes, make sure that it's working properly with the new imported function. The tests here will tell you if everything is hooked up correctly.
Thanks.
As you mentioned, I've implemented those changes, and I ran all the tests locally on my device. They are all passing successfully. Could you please review the changes?
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 69.62%. Comparing base (
3b6084f
) to head (f3b31dd
). Report is 26 commits behind head on master.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hey @atharva-2001 and Prof. @wkerzendorf, I've made the necessary changes to both sdec_plot.py and test_sdec_plot.py files as part of refactoring the parse_species_list functionality. The function is now correctly imported from util.py after being moved from liv_plot.py. Could you please review the changes and let me know if everything looks good or if any further adjustments are needed?
:pencil: Description
Type: :roller_coaster:
infrastructure
Relocated species list parsing logic from liv_plot.py to util.py to improve file organization and maintainability. No changes to functionality were made; the parsing logic remains the same but is now centralized in a more appropriate file.
:pushpin: Resources
:vertical_traffic_light: Testing
How did you test these changes?
:ballot_box_with_check: Checklist
build_docs
label