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.

Sign up for GitHub

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

Jump to bottom

Documentation Issues #190

Open
phistep opened this issue Nov 7, 2024 · 1 comment · May be fixed by #191
Open

Documentation Issues #190

phistep opened this issue Nov 7, 2024 · 1 comment · May be fixed by #191

Comments

@phistep
Copy link

phistep commented Nov 7, 2024 •
edited
Loading

In docs/userguide/variogramm.rst under The spherical model, there seems to be some inconsistency:

The default for Variogram is Trust-Region Reflective (TRF), which is also the default for Variogram.

which doesn't make any sense. Obviously the default for something is also the default for itself. What was the actual intention of the sentence? Which class shares teh default with Variogram?

the Kringing docs

state that

scikit-gstat/skgstat/Kriging.py

Lines 86 to 87 in 21ef923

solver : str
Do not change this argument

without any further explanation. Yet from looking at the code

it seems to me that scipy and numpy are both well supported options?

scikit-gstat/skgstat/Kriging.py

Lines 258 to 268 in 21ef923

@solver.setter
def solver(self, value):
if value == 'numpy':
self._solve = numpy_solve
elif value == 'scipy':
self._solve = scipy_solve
elif value == 'inv':
self._solve = inv_solve
else:
raise AttributeError("solver has to be ['inv', 'numpy', 'scipy']")
self._solver = value

The website link on Github sends one to https://mmaelicke.github.io/scikit-gstat/, which is not set up.

image

Maybe set-up Github pages for this repo or change the link to point to https://scikit-gstat.readthedocs.io. I was first discouraged to use this package as the first link I clicked to interact was broken. It might also be nice to add the link to the docs to PyPI (done in the PR)

note to self: intersphinx mapping for gstools is broken:

scikit-gstat/docs/conf.py

Line 219 in 21ef923

'gstools': ('https://geostat-framework.readthedocs.io/projects/gstools/en/latest/', None),

scikit-gstat/docs/tutorials/tutorial_01_getting_started.py

Line 140 in 21ef923

# **Note:** The recommended way for kriging applications is to use the interface to :any:`gstools`.

https://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html

the achor would resolve on https://geostat-framework.readthedocs.io/projects/gstools/en/stable/api.html (instead of package.html) ... how to fix this?

The order of tutorials is wrong, the RST seems to be auto-generated from the Code Cells

image

I'm also collecting more fixes in #191
@mmaelicke
Copy link
Owner

nice. You are absolutely right. I guess I read over that part a million times and never spotted this. All I want to say here is, that the Variogram uses TRF as a default, so I guess we can remove the sentence from 'which' on.

Would you add that fix to your #191 PR?

@mmaelicke mmaelicke linked a pull request Nov 7, 2024 that will close this issue
Documentation Fixes #191
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Documentation Fixes phistep/scikit-gstat
2 participants
@phistep @mmaelicke