ioos / ioos_qc

:ballot_box_with_check: :ocean: IOOS QARTOD and other Quality Control tests implemented in Python
https://ioos.github.io/ioos_qc/
Apache License 2.0
46 stars 27 forks source link

Added required packages for building docs #124

Closed Sakshamgupta90 closed 2 months ago

Sakshamgupta90 commented 3 months ago

Hi @ocefpaf, is this PR fine?

ocefpaf commented 3 months ago

Hi @ocefpaf, is this PR fine?

@Sakshamgupta90 the tests are failing. You can click on the links above to inspect the logs. Please let me know what you find. You should familiarize yourself with Continuous Integration systems (CI) and running tests at every PR. It is good practice to always run the tests and ensure things are passing.

PS: You may also find that you may need to update your branch in order to get the latest conde changes in this branch. It is usually done via rebasing. Let me know if you need help with that.

Sakshamgupta90 commented 3 months ago

Hi @ocefpaf, is this PR fine?

@Sakshamgupta90 the tests are failing. You can click on the links above to inspect the logs. Please let me know what you find. You should familiarize yourself with Continuous Integration systems (CI) and running tests at every PR. It is good practice to always run the tests and ensure things are passing.

PS: You may also find that you may need to update your branch in order to get the latest conde changes in this branch. It is usually done via rebasing. Let me know if you need help with that.

Hi, I am currently looking and understanding about these tests. And yes my bad, these packages are already mentioned in the requirements file.

ocefpaf commented 2 months ago

@Sakshamgupta90 this PR is failing b/c of the numpy version. You can verify that by inspecting the logs. You did fix it in #125. That means you need to rebase this branch. A you familiar with that rebasing?

ocefpaf commented 2 months ago

@Sakshamgupta90 do you plan to tackle the documentation build warnings on this PR or in a fresh one?

Sakshamgupta90 commented 2 months ago

@Sakshamgupta90 do you plan to tackle the documentation build warnings on this PR or in a fresh one?

Hi @ocefpaf, I did it on the same PR.

ocefpaf commented 2 months ago

I did it on the same PR.

@Sakshamgupta90 there is a single commit that adds pooch. If you try to build the docs, even with this PR checked out, you will see many warnings. We touched based on those via slack before. By fixing them you will force yourself to navigate the docs and, hopefully, get more familiar with ioos_qc.

Here is what you can do, enter the docs directory and try to build them with linkchecker:

make clean html linkcheck

Here is one of the warnings:

Args:
----
:20: (ERROR/3) Unexpected indentation.
:23: (WARNING/2) Block quote ends without a blank line; unexpected unindent.
:29: (WARNING/2) Title underline too short.

That means a docstring is poorly formatted and require some editing.

Sakshamgupta90 commented 2 months ago

I did it on the same PR.

@Sakshamgupta90 there is a single commit that adds pooch. If you try to build the docs, even with this PR checked out, you will see many warnings. We touched based on those via slack before. By fixing them you will force yourself to navigate the docs and, hopefully, get more familiar with ioos_qc.

Here is what you can do, enter the docs directory and try to build them with linkchecker:

make clean html linkcheck

Here is one of the warnings:

Args:
----
:20: (ERROR/3) Unexpected indentation.
:23: (WARNING/2) Block quote ends without a blank line; unexpected unindent.
:29: (WARNING/2) Title underline too short.

That means a docstring is poorly formatted and require some editing.

Hi @ocefpaf, I was trying the edit the docstring, and I was able to fix the warning :29: (WARNING/2) Title underline too short. But still the rest of the following errors, keep on arising. Is there some other way that shows where exactly it shows the indentation error? I even tried to rewriting the docstring from scratch but still no positive result displayed.

:20: (ERROR/3) Unexpected indentation.
:23: (WARNING/2) Block quote ends without a blank line; unexpected unindent.
ocefpaf commented 2 months ago

@Sakshamgupta90 it is a wack-a-mole game, what I do is to comment out everything but one function and build the docs, fix it, and uncomment the next one.

Sakshamgupta90 commented 2 months ago

@Sakshamgupta90 it is a wack-a-mole game, what I do is to comment out everything but one function and build the docs, fix it, and uncomment the next one.

Hi @ocefpaf, so I was trying to do the same. And notice that, while building docs it gives errors like unexpected indentation, and so on but if I look into pre-commit errors, they are completely different. So, if I try to fix the pre-commit errors, then should it works?

Sakshamgupta90 commented 2 months ago

Hi @ocefpaf, resolved all the warnings and errors, shall I create a new PR of the formatted docstring? or shall I commit in the same?

ocefpaf commented 2 months ago

Hi @ocefpaf, resolved all the warnings and errors, shall I create a new PR of the formatted docstring? or shall I commit in the same?

You should commit those in this PR. Please let me know when you are ready.

Sakshamgupta90 commented 2 months ago

Hi @ocefpaf, resolved all the warnings and errors, shall I create a new PR of the formatted docstring? or shall I commit in the same?

You should commit those in this PR. Please let me know when you are ready.

Yes, I am ready to commit.

ocefpaf commented 2 months ago

Yes, I am ready to commit.

By ready I meant ready with the PR. You still have a single commit there that doesn't include the doc fixes.

Sakshamgupta90 commented 2 months ago

Hi @ocefpaf, I implemented the docstring as per the numpy style guide. this is how it looks, is this ok?


"""Checks that the calculated speed between two points is within reasonable bounds.

    This test calculates a speed between subsequent points by
      * using latitude and longitude to calculate the distance between points
      * calculating the time difference between those points
      * checking if distance/time_diff exceeds the given threshold(s)

    Missing and masked data is flagged as UNKNOWN.

    If this test fails, it typically means that either a position or time is bad data,
    or that a platform is mislabeled.

    Ref: ARGO QC Manual: 5. Impossible speed test

    Parameters
    ----------
    lon
        Longitudes as a numeric numpy array or a list of numbers.
    lat
        Latitudes as a numeric numpy array or a list of numbers.
    tinp 
        Time data as a sequence of datetime objects compatible with pandas DatetimeIndex.
        This includes numpy datetime64, python datetime objects and pandas Timestamp object.
        ie. pd.DatetimeIndex([datetime.utcnow(), np.datetime64(), pd.Timestamp.now()])
        If anything else is passed in the format is assumed to be seconds since the unix epoch.
    suspect_threshold  
        A float value representing a speed, in meters per second.
        Speeds exceeding this will be flagged as SUSPECT.
    fail_threshold 
        A float value representing a speed, in meters per second.
        Speeds exceeding this will be flagged as FAIL.

    Returns
    -------
    flag_arr
        A masked array of flag values equal in size to that of the input.

    """
Sakshamgupta90 commented 2 months ago

Hi @ocefpaf, you merged the wrong one. I havent commit the new docstring yet. I just asked for the confirmation. Shall I raise a new PR for that?

Hi @ocefpaf, I implemented the docstring as per the numpy style guide. this is how it looks, is this ok?

"""Checks that the calculated speed between two points is within reasonable bounds.

    This test calculates a speed between subsequent points by
      * using latitude and longitude to calculate the distance between points
      * calculating the time difference between those points
      * checking if distance/time_diff exceeds the given threshold(s)

    Missing and masked data is flagged as UNKNOWN.

    If this test fails, it typically means that either a position or time is bad data,
    or that a platform is mislabeled.

    Ref: ARGO QC Manual: 5. Impossible speed test

    Parameters
    ----------
    lon
        Longitudes as a numeric numpy array or a list of numbers.
    lat
        Latitudes as a numeric numpy array or a list of numbers.
    tinp 
        Time data as a sequence of datetime objects compatible with pandas DatetimeIndex.
        This includes numpy datetime64, python datetime objects and pandas Timestamp object.
        ie. pd.DatetimeIndex([datetime.utcnow(), np.datetime64(), pd.Timestamp.now()])
        If anything else is passed in the format is assumed to be seconds since the unix epoch.
    suspect_threshold  
        A float value representing a speed, in meters per second.
        Speeds exceeding this will be flagged as SUSPECT.
    fail_threshold 
        A float value representing a speed, in meters per second.
        Speeds exceeding this will be flagged as FAIL.

    Returns
    -------
    flag_arr
        A masked array of flag values equal in size to that of the input.

    """
ocefpaf commented 2 months ago

Hi @ocefpaf, you merged the wrong one. I havent commit the new docstring yet. I just asked for the confirmation. Shall I raise a new PR for that?

No problem. You can send an amended in a fresh branch. I wanted to get pooch in there so others can build the docs too.