search

CNES / pangeo-pytide

Tidal constituents analysis in Python.
BSD 3-Clause "New" or "Revised" License
31 stars 10 forks source link

pytide, suggestions #1

Closed apatlpo closed 4 years ago

apatlpo commented 5 years ago

This PR aims at discussing several suggestions for pytide:

[ ] = to be done [X] = done [0] = not addressed in this PR

I/O

[0] support numpy arrays and xarrays for inputs, output xarrays

[0] support standard datetime formats

[X] harmonic_analysis one should either 1/ pass an epoch directly (and automatically compute nodal corrections) 2/ pass nodal correction

core

[X] add repr method to WaveTable

[X] avoid transposition of nodal constituents

[0] is compute_nodal_correction accurate? greenwich arguments are not nodal corrections I believe: I would rather say compute_astronomical_arguments

doc

[X] correct call totide method and install command

[ ] load data from an opendap server rather than from local file in template example

[0] use of xarray in template calculation

[0] clarify time formats

[0] clarify meaning of complex amplitudes

[0] add links to other libraries for tidal analysis

Fred: please tell me if you think this is a relevant approach for these suggestions

apatlpo commented 5 years ago

I gave a try to pytide a first try and am updating the list above

fbriol commented 5 years ago

Well for the moment I'm the only user, so it works;)

Indeed, the first version does not process Xarray data. But we can add it, but I need to understand how this will simplify your task. For the moment I have not understood correctly.

NaN values are masked data. To see how to take them into account.

Complex values represent an angle. So:

The object had a tide method, not anymore (I modified the README). It has been replaced by two methods. tide_from_time_series and tide_from_mapping. The first method allows calculating the tide for a time series tide=f(t) and the second for a mapping tide=(lon, lat). Not sure I documented it.

I'll check your notebook. But I'm not going to have much time until next Thursday.

fbriol commented 5 years ago

In the meantime, I've modified your notebook to calculate your tide correctly : https://anaconda.org/fbriol/pytide_aurelien/notebook

apatlpo commented 5 years ago

I pushed a first commit but am unable to test it as I can't succeed to install the library locally:

(sandbox) br146-058:pangeo-pytide aponte$ python setup.py install
running install
running bdist_egg
running egg_info
writing src/pytide.egg-info/PKG-INFO
writing dependency_links to src/pytide.egg-info/dependency_links.txt
writing requirements to src/pytide.egg-info/requires.txt
writing top-level names to src/pytide.egg-info/top_level.txt
reading manifest file 'src/pytide.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
warning: no previously-included files matching '*.py[dco]' found anywhere in distribution
warning: no previously-included files matching '*.so' found anywhere in distribution
no previously-included directories found matching 'docs/build'
writing manifest file 'src/pytide.egg-info/SOURCES.txt'
installing library code to build/bdist.macosx-10.9-x86_64/egg
running install_lib
running build_py
copying src/pytide/version.py -> build/lib.macosx-10.9-x86_64-3.7/pytide
copying src/pytide/__init__.py -> build/lib.macosx-10.9-x86_64-3.7/pytide
running build_ext
Traceback (most recent call last):
  File "setup.py", line 298, in <module>
    main()
  File "setup.py", line 294, in main
    zip_safe=False)
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/site-packages/setuptools/__init__.py", line 145, in setup
    return distutils.core.setup(**attrs)
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/distutils/dist.py", line 966, in run_commands
    self.run_command(cmd)
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/distutils/dist.py", line 985, in run_command
    cmd_obj.run()
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/site-packages/setuptools/command/install.py", line 67, in run
    self.do_egg_install()
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/site-packages/setuptools/command/install.py", line 109, in do_egg_install
    self.run_command('bdist_egg')
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/distutils/cmd.py", line 313, in run_command
    self.distribution.run_command(command)
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/distutils/dist.py", line 985, in run_command
    cmd_obj.run()
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/site-packages/setuptools/command/bdist_egg.py", line 172, in run
    cmd = self.call_command('install_lib', warn_dir=0)
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/site-packages/setuptools/command/bdist_egg.py", line 158, in call_command
    self.run_command(cmdname)
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/distutils/cmd.py", line 313, in run_command
    self.distribution.run_command(command)
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/distutils/dist.py", line 985, in run_command
    cmd_obj.run()
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/site-packages/setuptools/command/install_lib.py", line 11, in run
    self.build()
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/distutils/command/install_lib.py", line 107, in build
    self.run_command('build_ext')
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/distutils/cmd.py", line 313, in run_command
    self.distribution.run_command(command)
  File "/Users/aponte/.miniconda3/envs/sandbox/lib/python3.7/distutils/dist.py", line 985, in run_command
    cmd_obj.run()
  File "setup.py", line 147, in run
    self.build_cmake(ext)
  File "setup.py", line 212, in build_cmake
    ] + self.set_cmake_user_options()
  File "setup.py", line 192, in set_cmake_user_options
    result.append(self.eigen())
  File "setup.py", line 162, in eigen
    "Unable to find the Eigen3 library in the conda distribution "
RuntimeError: Unable to find the Eigen3 library in the conda distribution used.
(sandbox) br146-058:pangeo-pytide aponte$
fbriol commented 5 years ago

You have to install the conda package "eigen".

apatlpo commented 5 years ago

@fbriol: please check my attempt at overriding compute_nodal_corrections, it is not functional now, see the updated gist. if you have any ideas what's wrong ...

apatlpo commented 5 years ago

One issue I have with the current implementations is that you rely on inheritance whose details are difficult to understand for me (and many other scientists). Composition would have been easier.

fbriol commented 5 years ago

If we compose this class, the code will just be a little different. Instead of writing "super().method" we will write, "self.wt_.method()". I'm not sure that this makes the code any clearer...

Le dim. 29 sept. 2019 à 14:37, Aurélien Ponte notifications@github.com a écrit :

One issue I have with the current implementations is that you rely on inheritance whose details I (and many other scientists) are difficult to understand. Composition would have been easier.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CNES/pangeo-pytide/pull/1?email_source=notifications&email_token=AADBASR3VTPVJIUT4OMY2VLQMCOP7A5CNFSM4I22MTOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD73TUJY#issuecomment-536295975, or mute the thread https://github.com/notifications/unsubscribe-auth/AADBASRIJYN7D7SW3JBM7JDQMCOP7ANCNFSM4I22MTOA .

apatlpo commented 5 years ago

no pb, I just wanted to let you know inheritance is difficult for scientists to navigate around.

Could you please look at cell 8 in the updated notebook and tell me what is going wrong?

fbriol commented 4 years ago

Change the line super(core.WaveTable, self).compute_nodal_corrections(time) to super(WaveTable, self).compute_nodal_corrections(time)or super().compute_nodal_corrections(time)

apatlpo commented 4 years ago

Ok, I've done as much as I can afford to for now. There is a list of improvements that would be nice to have above. Unfortunately I cannot address them for now. Being able to handle xarray would be the most important one I think. @fbriol please tell me what you think

apatlpo commented 4 years ago

I forgot to mention the gist was updated

apatlpo commented 4 years ago

@fbriol , do you think you'll be have the time to look at this sometime? thx

fbriol commented 4 years ago

I tried to take into account all of your changes. I implemented them differently. You have a WaveDict class that manages the wavetable like a dictionary. This, in my opinion, provides a clearer interface. I have not yet integrated XArray. I close this PR,