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

Fix field plotting #1247

Closed
wants to merge 14 commits into from
Closed

Fix field plotting #1247

wants to merge 14 commits into from

Conversation

CKehl
Copy link
Contributor

@CKehl CKehl commented Sep 27, 2022

Added mini-test for loading and plotting NEMO data with dask for error replication and fix #741

Comment: little clean-up of open branches. This one is a small addition adding more stability to the plotting by checking that plotting works correctly. Valuable addition with little extra hassle - hence: merge-request.

@CKehl CKehl self-assigned this Sep 28, 2022
@CKehl CKehl added the testing label Sep 28, 2022
Comment on lines 74 to 75
# ax = fig.gca(projection='3d')
ax = fig.gca()
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change. I think this is what breaks the unit-tests. The ax is now not set as 3D anymore, so there is no set_zlevel method. Revert back to original?

Copy link
Contributor Author

@CKehl CKehl Oct 3, 2022

Choose a reason for hiding this comment

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

The change was necessary cause that line broke bccb214. In short: in Windows, the workflow obtains a newer version of matplotlib, and in matplotlib 3.6.0, the function figure.gca() has no projection parameters. Unexpected change, but therefore I needed to make this projection option part of the figure creation procedure. I guess help would be welcome from bccb214 onwards.

@@ -28,4 +28,5 @@ dependencies:
- nbval
- scikit-learn
- pykdtree
- cartopy
Copy link
Member

Choose a reason for hiding this comment

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

Why is cartopy now need? That was not the case before, and I suggest not to keep it

Suggested change
- cartopy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that will make the new test fail that actually tests platecarree plotting

- cftime>=1.3.1
- ipykernel
- pytest
- nbval
- pykdtree
- cartopy
Copy link
Member

Choose a reason for hiding this comment

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

See above, I suggest not to make cartopy an explicit dependency (as we had not done before)

Suggested change
- cartopy

@@ -101,7 +102,7 @@ def plotparticles(particles, with_particles=True, show_time=None, field=None, do


def plotfield(field, show_time=None, domain=None, depth_level=0, projection='PlateCarree', land=True,
vmin=None, vmax=None, savefile=None, **kwargs):
vmin=None, vmax=None, savefile=None, use3D=False, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Is this variable still needed now? I am a bit confused what this does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

possibly not, indeed

parcels/plotting.py Outdated Show resolved Hide resolved
Comment on lines +1 to +54
from parcels import FieldSet, ParticleSet, JITParticle, AdvectionRK4, ErrorCode
from datetime import timedelta as delta
import numpy as np
import dask.array as da # NOQA
from os import path
from matplotlib import pyplot as plt # NOQA
import pytest


def periodicBC(particle, fieldset, time):
if particle.lon > 180:
particle.lon -= 360


def test_field_from_netcdf():
data_path = path.join(path.dirname(__file__), 'test_data/')

filenames = {'U': {'lon': data_path + 'mask_nemo_cross_180lon.nc',
'lat': data_path + 'mask_nemo_cross_180lon.nc',
'data': data_path + 'Uu_eastward_nemo_cross_180lon.nc'},
'V': {'lon': data_path + 'mask_nemo_cross_180lon.nc',
'lat': data_path + 'mask_nemo_cross_180lon.nc',
'data': data_path + 'Vv_eastward_nemo_cross_180lon.nc'}
}
variables = {'U': 'U',
'V': 'V'}
dimensions = {'lon': 'glamf', 'lat': 'gphif'}
return FieldSet.from_netcdf(filenames, variables, dimensions, interp_method='cgrid_velocity', chunksize='auto', allow_time_extrapolation=True)


@pytest.fixture(name="fieldset")
def fieldset_fixture():
return test_field_from_netcdf()


def test_pset_create_field(fieldset, npart=100):
lonp = -180 * np.ones(npart)
latp = [i for i in np.linspace(-70, 88, npart)]
pset = ParticleSet.from_list(fieldset, JITParticle, lon=lonp, lat=latp)
return pset


def DeleteParticle(particle, fieldset, time):
particle.delete()


if __name__ == '__main__':
fset = test_field_from_netcdf()
print(fset)
pset = test_pset_create_field(fset)
kernels = pset.Kernel(AdvectionRK4) + periodicBC
pset.execute(kernels, dt=delta(hours=1), output_file=None,
recovery={ErrorCode.ErrorOutOfBounds: DeleteParticle})
pset.show(field=fset.U)
Copy link
Member

Choose a reason for hiding this comment

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

What does this new set of functions test that was not tested before? Why is it needed?

CKehl and others added 2 commits October 5, 2022 10:34
Co-authored-by: Erik van Sebille <e.vansebille@uu.nl>
@erikvansebille
Copy link
Member

I think that this was now fixed in 0326f77

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"compute_on_defer" not working with dask array
2 participants