Installation instructions

rweigel commented 1 month ago

I executed the two commands at

https://github.com/amisr/amisrsynthdata/tree/main?tab=readme-ov-file

pip install amisrsynthdata
amisrsynthdata config.yaml

and received an error related to apexpy not being installed. Perhaps warn the user that the second command will fail unless they do pip install amisrsynthdata[apex]?

At https://amisrsynthdata.readthedocs.io/en/latest/installation.html, the command pin install appears several times. Should that be pip install?

rweigel commented 1 month ago

The README gives

pip install amisrsynthdata[apex]

but I needed

pip install 'amisrsynthdata[apex]'

rweigel commented 1 month ago

git clone https://github.com/amisr/amisrsynthdata.git
cd amisrsynthdata
pip install -e .

gave

ERROR: File "setup.py" or "setup.cfg" not found. Directory cannot be installed in editable mode: /private/tmp/amisrsynthdata
(A "pyproject.toml" file was found, but editable mode currently requires a setuptools-based build.)

ljlamarche commented 2 weeks ago

Hi @rweigel , thank you for your comments! I've published a new release of this package to pypi that fixes many of these installation issues. The one caveat is the apexpy dependency has some version incompatibilities (https://github.com/aburrell/apexpy/issues/134, https://github.com/aburrell/apexpy/issues/135) that aren't currently properly accounted for. Sometime next week, I think we'll do a new minor release of apexpy with required versions pinned, which should address the issue of amirsynthdata not installing easily with pip. For now, if you'd like to try reinstalling, please do the following in python 3.11 or lower:

pip install numpy==1.26.4
pip install 'amisrsynthdata[plots]'

I will post here when the new version of apexpy has been published and you can try installing with the normal instructions.

rweigel commented 1 week ago

I was able to install, but wanted to execute something like

amisrsynthdata config.yaml

but it seems I need to use the pip install -e ... example to access this file. After doing so, I was not able to find config.yaml, but did find example_synth_config.yaml. I recommend clarification in the documentation. Perhaps set this file as the default when amisrsynthdata is executed and inform the user that it is reading the default and print the location of the default. This will also make the user not need to do the pip install -e - they can execute amisrsynthdata, see the path of the default, copy it locally and edit it, and then run again with the modified file.

As a reviewer (and user), I would prefer to not spend most of my time dealing with installation and packaging issues before getting something working. I recommend adding a sequence of commands in the documentation that will 'just work' and for which statements such as 'This assumes you already have apexpy installed' are not needed. (See the "don't make me think" principle.)

One minor recommendation. amisrsynthdata example_synth_config.yaml gives the feedback

/opt/miniconda3/envs/python3.11.9/lib/python3.11/site-packages/cartopy/io/__init__.py:241: DownloadWarning: Downloading: https://naturalearth.s3.amazonaws.com/10m_physical/ne_10m_coastline.zip
  warnings.warn(f'Downloading: {url}', DownloadWarning)

which is useful, but I am not sure why this is a warning. Also, after the program completed, no feedback was given and my first guess was it failed. However, several files were created in the repo. I suggest giving more feedback to the user.

Also, I sure if there is a way of handling this, but I tried the commands without checking my Python version (which was 3.12) and after about 30 seconds received an error. Is there a way to abort the installation early if there is a python version incompatibility?

ljlamarche commented 1 week ago

Thanks you for the feedback on the installation issues! I've updated the documentation to provide clearer starting instructions for new users. I've published a new version (v1.1.8) to pypi, which should link to the "stable" (default) documentation on ReadTheDocs. Additionally, a new version of apexpy has been published with versions pinned so pip installation should "just work" now. Can you please try the new instructions and let me know if you have issues?

I'm hesitant include the example configuration file in the package because it doesn't make a lot of sense for this code and it's application to have a "default". There isn't really a single background ionospheric state or radar mode that is "typical", and I worry that introducing users to the package with a built-in configuration file but requiring they provide a different one as soon as they start doing anything "real" with the package would be confusing. I have modified the documentation to include more specific instruction of how to download and use the example configuration file.

The warning you identified comes from cartopy - it looks like it's just informing you that it is downloading coastline data for the plots. I believe this is standard for cartopy and I'm not sure I should suppress the warning in amisrsynthdata.

Unfortunately, I'm not sure there is a way to check the python version before building the package and save yourself the 30 seconds of failed installation time... This seems to be a feature of pip and I can't find any flag or setting to check the python version first. I have pinned python<3.12 in both amisrsynthdata and apexpy so it's evident on the pypi page at least. I know we're hoping to fix the apexpy compatibility issue sometime before the end of the year.

rweigel commented 5 days ago

I have finished my checklist.

The following notes are for the benefit of the author and do not need a reply or action.

See https://stackoverflow.com/questions/19534896/enforcing-python-version-in-setup-py regarding Python version checking. I put version checks in my code when I know something will fail because I've found many users conclude "code does not work" and give up and move on when a pip install does not work. I don't regard this as mandatory, however. Probably this should be handled in ApexPy.

https://amisrsynthdata.readthedocs.io/en/stable/installation.html# has pip install amisrsynthdata[plots], which does not work for me without quotes.

I was able to install using pip install 'amisrsynthdata[plots]'

Other optional edits to consider:

python -> Python in manuscript.
I usually use curl -O instead of wget in my examples because I've found curl always installed already on Linux, Mac, and (more recently) Windows, whereas wget usually requires an install.
At https://amisrsynthdata.readthedocs.io/en/stable/#example-usage, docs/synthdata_summary_ne.png docs/synthdata_gemini_plot.png show a broken image.
Add links where "SRI data format" is mentioned (at least twice) because a Google search on "SRI data format" does not turn up anything relevant on the first page.
Link to your suggestion for ApexPy install issues in the example usage section
Link to the JOSS paper at https://amisrsynthdata.readthedocs.io/en/stable/# (When I am writing a paper, I always appreciate not having to spend a lot of time figuring out how to cite software and data.)
Remove empty page (or was there a generation error)? https://amisrsynthdata.readthedocs.io/en/stable/reference.html
Is https://amisrsynthdata.readthedocs.io/en/stable/usage.html#syntheticdata-class-example expected to work with the example config? I get fov_glat = sd.Geomag['Latitude'] AttributeError: 'SyntheticData' object has no attribute 'Geomag'
I find it helpful if a link to the GitHub repo is prominent in documentation, for example at https://amisrsynthdata.readthedocs.io/en/stable/index.html. I typically read the docs and scan the source before attempting to use something, so cross-links are appreciated.
Example config and test config are similar. It may help reduce future maintenance if only test file used as example.
Link to tutorial notebook in https://amisrsynthdata.readthedocs.io/en/stable/index.html#example-usage
About 1/2 of page at https://github.com/amisr/amisrsynthdata/blob/main/tutorial.ipynb is warnings.

I doubt this is worth fixing, but on an older intel mac 10.15 with Python 3.10 I got the following (I tend to not upgrade OS on systems with many difficult-to install packages, such as ApexPy, because an upgrade means days of figuring out how to get the packages working again).

gfortran --version
GNU Fortran (Homebrew GCC 9.3.0_1) 9.3.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

pip install 'amisrsynthdata[plots]'
Collecting amisrsynthdata[plots]
  Downloading amisrsynthdata-1.1.8-py3-none-any.whl (107 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 107.1/107.1 kB 3.8 MB/s eta 0:00:00
Collecting pymap3d
  Downloading pymap3d-3.1.0-py3-none-any.whl (60 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 60.6/60.6 kB 3.6 MB/s eta 0:00:00
Collecting apexpy
  Downloading apexpy-2.0.2.tar.gz (331 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 331.4/331.4 kB 10.7 MB/s eta 0:00:00
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... error
  error: subprocess-exited-with-error

  × Preparing metadata (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> [16 lines of output]
      + meson setup /private/var/folders/pr/6xm0lrgx50g4276btj3p_dh00000gn/T/pip-install-l8vlg7vw/apexpy_7916eb0a5b27429f9b30aee37f30ec3b /private/var/folders/pr/6xm0lrgx50g4276btj3p_dh00000gn/T/pip-install-l8vlg7vw/apexpy_7916eb0a5b27429f9b30aee37f30ec3b/.mesonpy-qpwie1xo -Dbuildtype=release -Db_ndebug=if-release -Db_vscrt=md --native-file=/private/var/folders/pr/6xm0lrgx50g4276btj3p_dh00000gn/T/pip-install-l8vlg7vw/apexpy_7916eb0a5b27429f9b30aee37f30ec3b/.mesonpy-qpwie1xo/meson-python-native-file.ini
      The Meson build system
      Version: 1.6.0
      Source dir: /private/var/folders/pr/6xm0lrgx50g4276btj3p_dh00000gn/T/pip-install-l8vlg7vw/apexpy_7916eb0a5b27429f9b30aee37f30ec3b
      Build dir: /private/var/folders/pr/6xm0lrgx50g4276btj3p_dh00000gn/T/pip-install-l8vlg7vw/apexpy_7916eb0a5b27429f9b30aee37f30ec3b/.mesonpy-qpwie1xo
      Build type: native build
      Project name: apexpy
      Project version: 2.0.2
      C compiler for the host machine: cc (clang 12.0.0 "Apple clang version 12.0.0 (clang-1200.0.32.29)")
      C linker for the host machine: cc ld64 609.8
      Host machine cpu family: x86_64
      Host machine cpu: x86_64

      ../meson.build:17:0: ERROR: Compiler gfortran cannot compile programs.

      A full log can be found at /private/var/folders/pr/6xm0lrgx50g4276btj3p_dh00000gn/T/pip-install-l8vlg7vw/apexpy_7916eb0a5b27429f9b30aee37f30ec3b/.mesonpy-qpwie1xo/meson-logs/meson-log.txt
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
error: metadata-generation-failed

× Encountered error while generating package metadata.
╰─> See above for output.

note: This is an issue with the package mentioned above, not pip.
hint: See above for details.

ljlamarche commented 4 days ago

@rweigel , thank you for your thorough feedback! I will work to fix these point by point. The documentation and linking issues can definitely be corrected, I appreciate you identifying them.

The dependency on the finicky apexpy is an unfortunate necessity. I really need a magnetic coordinate package to handle the plasma velocity correctly, and apexpy has certain capabilities that tend to be lacking from the alternative options (most of which are equally if not more fraught with installation issues). I will log the issues you have had and we will try to fix them in the next release of apexpy.

amisr / amisrsynthdata

Installation instructions #17