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

Process pointinspection and Documentation #271

Merged
merged 26 commits into from
Dec 15, 2018
Merged

Process pointinspection and Documentation #271

merged 26 commits into from
Dec 15, 2018

Conversation

nilshempelmann
Copy link
Member

@cehbrecht I need help with the workdir settings. If I get it right it needs to be updated in all the eggshell functions as well, since there are many files produced there.

@huard @Zeitsperre If you have time check the Subset Process Documentation.

@nilshempelmann
Copy link
Member Author

missed to commit changes in eggshell

@cehbrecht
Copy link
Member

travis and codacy are not happy yet. You could also add a test.

@cehbrecht
Copy link
Member

I need help with the workdir settings. If I get it right it needs to be updated in all the eggshell functions as well, since there are many files produced there.

I haven't checked the eggshell ... but it should provide supporting functions not knowing about self.workdir handling ... that is pywps only. So, you need to find the right set of function parameters.

Copy link
Contributor

@huard huard left a comment

Choose a reason for hiding this comment

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

My suggestion would be for all processes to return netCDF files. This way we're able to use process outputs as inputs to other processes.

We can then have a process to convert netCDF to CSV.

**Polygons**
Abbreviation of the appropriate polygon.

**Mosaic**
Copy link
Contributor

Choose a reason for hiding this comment

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

Confusing, since PointInspection has no mosaic argument.

@nilshempelmann
Copy link
Member Author

@huard agree with netcdf outputs. How to configure the output if the number of outputs can vary? Can you point me to an example?

@huard
Copy link
Contributor

huard commented Dec 10, 2018

@nilshempelmann Do you mean if the number of geometries is larger than one, or if the number of input files is larger than one ? I'll assume it's the former, and then OCGIS supports it out of the box, you just pass a list of geometries to OCGIS and make sure you specify aggregate=True. The idea is to create another dimension for the output where the index is the gemetry index. This allows you to store the lat, lon as well along this dimension.
NCPP/ocgis#459
https://ocgis.readthedocs.io/en/latest/appendix.html?highlight=geom_dim#netcdf-output

@nilshempelmann
Copy link
Member Author

nilshempelmann commented Dec 10, 2018

@huard The number of netCDF output files vary if input files are model results from different GCMs-RCMs. Input files can be belonging to different modelruns, the process are sorting them and process all different models separatly eggshell.nc.nc_utils.sort_by_filename(resource, historical_concatination=False)
(mulitiple files of the same Modelrun are currently aggregated, including also aggregation of historical and suitable rcp run of the same Model output). Its not recommendet to store multiple models into one netcdf file.

And yes multiple geometries / optional unify results in variation of the number of outputs. So the current solution was to archive all of them and output one singe archive.

We also should keep the DRS-nameing logig and respect the CMIP/Cordex archive standarts. Here they separate the geometries/domains in seperate files. eggshell.nc.nc_utils.drs_filename()

A following process (e.g. visualisation) starts with archive_extract, in case of input=tarfile.

If you have a solution to avoid the archive-tar would be nice.

@huard
Copy link
Contributor

huard commented Dec 10, 2018

I understand now. You are saying the the current output is a tar file, which allows us to bundle multiple output files together in one single ComplexOutput.

I think now is the time to implement a generic approach for multiple file output using the MetaLink standard.

See geopython/pywps#298
bird-house/emu#64 (comment)

I suggest we first test and discuss it in Emu then move the solution to this process.

@nilshempelmann
Copy link
Member Author

And here comes the issue #273 :-)

@nilshempelmann
Copy link
Member Author

nilshempelmann commented Dec 10, 2018

Following processes are sucessfully running, but test are failing (related bird-house/emu#70):
Subset_countries
subset_continets
pointinspection

@huard need your help to get the oranus subset processes in place. #274

@nilshempelmann
Copy link
Member Author

@cehbrecht Finally :-)
But subset countries and continents output path needs to be fixed in the tests (disabled for the moment)

Copy link
Member

@cehbrecht cehbrecht left a comment

Choose a reason for hiding this comment

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

short review ... not tested yet. Added comments to the code.

tests/test_eggshell.py Outdated Show resolved Hide resolved
while execution.status == 'ProcessAccepted':
execution.checkStatus(sleepSecs=1)
output_json = execution.processOutputs[0].reference
if output_json[:7] == 'file://':
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 if needed? In a test you have a fixed expectation.

Copy link
Member Author

@nilshempelmann nilshempelmann Dec 13, 2018

Choose a reason for hiding this comment

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

@huard That's an Ouranos process. I leave it to you to change it correctly

self.assertEqual(nc.subset_featureid, 'montreal_circles.43')
nc.close()

def test_subset_wfs_opendap_01_default_geoserver(self):
Copy link
Member

Choose a reason for hiding this comment

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

This test (and others) look quite complicated. Can it be changed? Test code should be simple, otherwise it is to likely to break as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

@huard That's an Ouranos process. I leave it to you to change it correctly


try:
from tests import test_wps_utils
except ImportError:
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 the try needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

@huard That's an Ouranos process. I leave it to you to change it correctly

docs/source/descriptions/subset.rst Outdated Show resolved Hide resolved
docs/source/descriptions/shapefilepreparation.rst Outdated Show resolved Hide resolved
tests/test_eggshell.py Outdated Show resolved Hide resolved
self.config = configparser.RawConfigParser()
if os.path.isfile('configtests.cfg'):
self.config.read('configtests.cfg')
else:
Copy link
Member

Choose a reason for hiding this comment

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

why if?

Copy link
Member Author

Choose a reason for hiding this comment

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

@huard That's an Ouranos process. I leave it to you to change it correctly

flyingpigeon/processes/wps_pointinspection.py Outdated Show resolved Hide resolved
@nilshempelmann
Copy link
Member Author

@cehbrecht : commented your change request.
Feel free to update the Ouranos processes

@nilshempelmann nilshempelmann merged commit 7395296 into master Dec 15, 2018
@cehbrecht cehbrecht added this to the 1.5 milestone May 17, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants