Jul fe netcdf #576
Jul fe netcdf #576
Conversation
|
The failing tests are not due to your commit, but an issue with Xhistogram/Dask: xgcm/xhistogram#27 Anyway, you probably want to add more to this PR before merging should be considered. |
Mainly n the Selafin file reader
|
Thanks for the PR. Do you have the chance to either include a small telemac file or somehow make it available online? Then it will be easier to write unit-tests for the new reader and the converter scripts. What extra dependencies are required for the telemac-converter? This script should probably go in the scripts dir, but maybe this can be made part of the reader if it does not take too long to run. I understand this is still in draft-mode, is there anything in particular you would like feedback on now? I can try to skim through the code the coming week, it would be helpful to have the original and converted files to test with in that case. |
|
I think the init is fine.
I have struggled with the other functions
Do you have a slack or something a bit more interactive to exchange files
and ideas?
…On Sat, Apr 10, 2021 at 8:05 PM Gaute Hope ***@***.***> wrote:
Thanks for the PR. Do you have the chance to either include a small
telemac file or somehow make it available online? Then it will be easier to
write unit-tests for the new reader and the converter scripts. What extra
dependencies are required for the telemac-converter? This script should
probably go in the scripts dir, but maybe this can be made part of the
reader if it does not take too long to run.
I understand this is still in draft-mode, is there anything in particular
you would like feedback on now? I can try to skim through the code the
coming week, it would be helpful to have the original and converted files
to test with in that case.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#576 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACJCEFRFNCNMN6ZNPOSSLHDTICOPNANCNFSM42SSVAXA>
.
--
======
Julien Moreau, PhD, MSc, MSc
The NW-Edge <http://nw-edge.org>
Basin Analyst - Independent researcher in geology and applied geophysics
Co-Founder of Plastic@Bay <http://www.plasticatbay.org> - Strategic
development and research leader
SAS rep - North West Scotland - Scotland & Hebrides Rep
<https://www.sas.org.uk/rep/julien-moreau/>
Breadalbane, Durness, Lairg
IV27 4PN United Kingdom
Mobile: +44 (0) 746 20 781 62
***@***.***
***@***.***
|
|
Hi,
we just set up a public slack here:
https://join.slack.com/t/opendrift-dev/shared_invite/zt-ozansc5h-AzMOOS9jOs~3CBihRR37Lw
…-- gaute
søn. 11. apr. 2021 kl. 09:50 skrev Julien Moreau ***@***.***>:
I think the init is fine.
I have struggled with the other functions
Do you have a slack or something a bit more interactive to exchange files
and ideas?
On Sat, Apr 10, 2021 at 8:05 PM Gaute Hope ***@***.***> wrote:
> Thanks for the PR. Do you have the chance to either include a small
> telemac file or somehow make it available online? Then it will be easier
to
> write unit-tests for the new reader and the converter scripts. What extra
> dependencies are required for the telemac-converter? This script should
> probably go in the scripts dir, but maybe this can be made part of the
> reader if it does not take too long to run.
>
> I understand this is still in draft-mode, is there anything in particular
> you would like feedback on now? I can try to skim through the code the
> coming week, it would be helpful to have the original and converted files
> to test with in that case.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#576 (comment)
>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/ACJCEFRFNCNMN6ZNPOSSLHDTICOPNANCNFSM42SSVAXA
>
> .
>
--
======
Julien Moreau, PhD, MSc, MSc
The NW-Edge <http://nw-edge.org>
Basin Analyst - Independent researcher in geology and applied geophysics
Co-Founder of ***@***.*** <http://www.plasticatbay.org> - Strategic
development and research leader
SAS rep - North West Scotland - Scotland & Hebrides Rep
<https://www.sas.org.uk/rep/julien-moreau/>
Breadalbane, Durness, Lairg
IV27 4PN United Kingdom
Mobile: +44 (0) 746 20 781 62
***@***.***
***@***.***
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#576 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAN367M3RLUAU26F4ZAWT3TIFIEDANCNFSM42SSVAXA>
.
|
| from os import path, environ, sep | ||
| from scipy.spatial import cKDTree | ||
| #Activating PYTEL | ||
| dir = environ['HOMETEL']+'{}scripts{}python3'.format(sep,sep) |
There was a problem hiding this comment.
Using environment variable (HOMETEL) is not a good solution for all users, can this be solved another way to work out of the box?
There was a problem hiding this comment.
This variable comes straight from Telemac configuration I don't really know how to improve that.
Or we have to request to duplicate their code
| #Activating PYTEL | ||
| dir = environ['HOMETEL']+'{}scripts{}python3'.format(sep,sep) | ||
| sys.path.append(path.join( path.dirname(sys.argv[0]),dir))#pytel path use environment variables | ||
| from data_manip.formats.selafin import Selafin |
There was a problem hiding this comment.
I believe this line can be added to environment.yml under the pip section for automatic installation of this package:
git://github.com/CNR-Engineering/PyTelTools.git#egg=pyteltools
There was a problem hiding this comment.
I am not sure pyteltools is actually developed by the Telemac team. It is unclear if the two projects are linked. I can ask
| @@ -6,11 +6,11 @@ | |||
| 'y_wind': {'valid_min': -50, 'valid_max': 50, 'units': 'm/s', | |||
| 'long_name': 'Component of wind along y-direction (northwards if ' | |||
| 'projection is lonlat/Mercator)'}, | |||
| 'x_sea_water_velocity': {'valid_min': -10, 'valid_max': 10, | |||
| 'x_sea_water_velocity': {'valid_min': -20, 'valid_max': 20, | |||
There was a problem hiding this comment.
This is in the meantime changed also in master, so should be removed from this PR
There was a problem hiding this comment.
ok not too sure how to do that
There was a problem hiding this comment.
I have tried to restore the original state with checkout -- Tell me if it looks better
There was a problem hiding this comment.
I see that the two unused file were deleted, which is good. There is one conflict due to the valid_min/max-limits, but I believe this can be resolved in the browser as seen below (but probably this has to be done by yourself?).
Then we also need to find a solution for installation without users needing to set environment variables. But it could be that this environment variable is not needed anyway(?)
Eventually we would also like to have a unit-test for this reader, though then one would need a stable thredds server with an available dataset. However, in the meantime, a placeholder test could be made to simply check that it works well to import this reader and the selafin-tool.
There was a problem hiding this comment.
ok I don't really know how to discard my version.
regarding the environmental variables they are needed in Telemac to run the python scripts and other things. It would not run without it.
For the test I have something from Telemac itself but since it has no coordinate and I wasn't sure about how to setup proj4 in that case, I gave up
|
This PR does not interfere with anything else, so perhaps we can just merge this now, and eventually try later to make the installation automatic. But you'd might like to insert your name as author in the the reader, after a common header as in other readers/files: |
added a scipy version test for query arguments (python >=3.8) added header
|
Then I can merge this one now, if ok by you? |
|
I think it looks workable for now? |
Just some typos wrong time axis dimension and
use of CRS (Coordinate Reference system) instead of SRS