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

🩹 Use a context manager when opening a nc dataset #848

Conversation

jsnel
Copy link
Member

@jsnel jsnel commented Oct 3, 2021

avoid file locking when calling xarray.open_dataset

Checklist

  • βœ”οΈ Passing the tests (mandatory for all PR's)
  • πŸ‘Œ Closes issue (mandatory for ✨ feature and 🩹 bug fix PR's)

Closes issues

closes #847

avoid file locking when calling xarray.open_dataset
@jsnel jsnel requested a review from a team as a code owner October 3, 2021 12:16
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Oct 3, 2021

Sourcery Code Quality Report

βœ… Β Merging this PR will increase code quality in the affected files by 0.84%.

Quality metrics Before After Change
Complexity 3.20 ⭐ 2.91 ⭐ -0.29 πŸ‘
Method Length 24.00 ⭐ 26.00 ⭐ 2.00 πŸ‘Ž
Working memory 7.80 πŸ™‚ 7.64 πŸ™‚ -0.16 πŸ‘
Quality 79.78% ⭐ 80.62% ⭐ 0.84% πŸ‘
Other metrics Before After Change
Lines 33 34 1
Changed files Quality Before Quality After Quality Change
glotaran/builtin/io/netCDF/netCDF.py 79.78% ⭐ 80.62% ⭐ 0.84% πŸ‘

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • πŸ™‚ good
  • 😞 poor
  • β›” very poor

The πŸ‘ and πŸ‘Ž indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2021

Binder πŸ‘ˆ Launch a binder notebook on branch jsnel/pyglotaran/refactor/847_avoid_file_locking_nc_files_when_loading

@sonarcloud
Copy link

sonarcloud bot commented Oct 3, 2021

Kudos, SonarCloud Quality Gate passed!Β  Β  Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2021

Benchmark is done. Checkout the benchmark result page.
Benchmark differences below 5% might be due to CI noise.

Benchmark diff v0.4.1 vs. main

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)

All benchmarks:

       before           after         ratio
     [21ba272a]       [fac27de9]
     <v0.4.1>                   
-      47.2Β±0.2ms       31.7Β±0.2ms     0.67  BenchmarkOptimize.time_optimize(False, False, False)
-       284Β±0.6ms       37.3Β±0.7ms     0.13  BenchmarkOptimize.time_optimize(False, False, True)
-      65.1Β±0.3ms       52.4Β±0.3ms     0.80  BenchmarkOptimize.time_optimize(False, True, False)
-      67.1Β±0.3ms       56.0Β±0.4ms     0.83  BenchmarkOptimize.time_optimize(False, True, True)
-      47.3Β±0.2ms       41.1Β±0.4ms     0.87  BenchmarkOptimize.time_optimize(True, False, False)
-       284Β±0.9ms        47.0Β±40ms     0.17  BenchmarkOptimize.time_optimize(True, False, True)
       64.7Β±0.5ms       63.6Β±0.2ms     0.98  BenchmarkOptimize.time_optimize(True, True, False)
         67.2Β±1ms         67.8Β±6ms     1.01  BenchmarkOptimize.time_optimize(True, True, True)
             178M             179M     1.01  IntegrationTwoDatasets.peakmem_create_result
             198M             199M     1.01  IntegrationTwoDatasets.peakmem_optimize
-         202Β±2ms          166Β±2ms     0.82  IntegrationTwoDatasets.time_create_result
-      4.31Β±0.02s       1.64Β±0.06s     0.38  IntegrationTwoDatasets.time_optimize

Benchmark diff main vs. PR

Parametrized benchmark signatures:

BenchmarkOptimize.time_optimize(index_dependent, grouped, weight)

All benchmarks:

       before           after         ratio
     [ca1a75ea]       [fac27de9]
      32.0Β±0.04ms       31.7Β±0.2ms     0.99  BenchmarkOptimize.time_optimize(False, False, False)
         37.2Β±1ms       37.3Β±0.7ms     1.00  BenchmarkOptimize.time_optimize(False, False, True)
       52.5Β±0.3ms       52.4Β±0.3ms     1.00  BenchmarkOptimize.time_optimize(False, True, False)
       56.1Β±0.4ms       56.0Β±0.4ms     1.00  BenchmarkOptimize.time_optimize(False, True, True)
       41.3Β±0.2ms       41.1Β±0.4ms     0.99  BenchmarkOptimize.time_optimize(True, False, False)
        46.8Β±40ms        47.0Β±40ms     1.00  BenchmarkOptimize.time_optimize(True, False, True)
       63.7Β±0.5ms       63.6Β±0.2ms     1.00  BenchmarkOptimize.time_optimize(True, True, False)
        68.8Β±50ms         67.8Β±6ms     0.99  BenchmarkOptimize.time_optimize(True, True, True)
             182M             179M     0.99  IntegrationTwoDatasets.peakmem_create_result
             199M             199M     1.00  IntegrationTwoDatasets.peakmem_optimize
          164Β±3ms          166Β±2ms     1.01  IntegrationTwoDatasets.time_create_result
        1.54Β±0.1s       1.64Β±0.06s     1.07  IntegrationTwoDatasets.time_optimize

@codecov
Copy link

codecov bot commented Oct 3, 2021

Codecov Report

Merging #848 (fac27de) into main (ca1a75e) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #848   +/-   ##
=====================================
  Coverage   84.9%   84.9%           
=====================================
  Files         77      77           
  Lines       4371    4372    +1     
  Branches     785     785           
=====================================
+ Hits        3711    3712    +1     
  Misses       521     521           
  Partials     139     139           
Impacted Files Coverage Ξ”
glotaran/builtin/io/netCDF/netCDF.py 78.9% <100.0%> (+1.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update ca1a75e...fac27de. Read the comment docs.

Copy link
Member

@s-weigand s-weigand left a comment

Choose a reason for hiding this comment

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

LGTM πŸ‘

@jsnel jsnel merged commit 7797bc9 into glotaran:main Oct 3, 2021
s-weigand added a commit to s-weigand/pyglotaran-extras that referenced this pull request Oct 21, 2021
jsnel pushed a commit to glotaran/pyglotaran-extras that referenced this pull request Oct 22, 2021
Adds new plot function to plot data and fitted traces for selected wavelengths. 

* ✨ Added 'plot_fit_overview' function to plot data and fit per wavelength

✨ Also added some convenience functions for data pre-processing

* ✨ Added 'wavelength_range' parameter so the user can select data to plot

* 🧹 Resticted DatasetConvertible objects to always be xr.Dataset

* ♻️ Factored out extraction of irf location to a seperate function

* πŸ‘Œ Plotted fits and data are now shifted by the irf location

* 🧹 Unified xarray typing style

* πŸ‘Œβ™»οΈ Added 'dataset_name' argument, use 'load_dataset' to load result from file
Ref.: glotaran/pyglotaran#848

* ♻️ Refactored load_data and result_dataset_mapping to work with DataArray

πŸ‘Œ result_dataset_mapping and thus also plot_data_and_fits and plot_fit_overview, no work with single file paths

* β™»οΈβœ¨ Made 'plot_fit_overview' work properly with unevenly spaced wavelegths

♻️ This required to remove the argument 'wavelength_range' and instead pass the wavelengths directly to 'plot_fit_overview' which gives the user more control. For convenience the function 'select_plot_wavelengths' was added, which provies the functionality of 'wavelength_range'

* ♻️ Moved 'plot_fit_overview' helper functions to plotting.utils

'maximum_coordinate_range' and 'add_unique_figure_legend'

* ✨ Added figsize  argument to plot_data_overview

* πŸ‘Œ Reexport 'select_plot_wavelengths' from plotting.data for convenience

* ✨ Added divide_by_scale parameter to divide data by dataset_scale

This needs a feature added in pyglotaran 0.5.0, for older results it will show a warning.

* πŸ‘Œ Made function name in 'select_plot_wavelengths' warning dynamic

* πŸ“š Added comments and docstrings to plot style code

* πŸ‘Œ Made the ylabel of plots an argument and default to 'a.u.'

* ♻️ Addressed requested renaming and moving functions suited for this PR

Ref: #39 (review)

* ♻️ Moved plot_concentrations to its own module with the same name
@jsnel jsnel deleted the refactor/847_avoid_file_locking_nc_files_when_loading branch November 6, 2022 23:16
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.

πŸ› Dataset file locked during lifetime of script
2 participants