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.

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

Added required packages for building docs #124

Merged
merged 2 commits into from
Aug 7, 2024

Conversation

Sakshamgupta90
Copy link
Contributor

No description provided.

@Sakshamgupta90
Copy link
Contributor Author

Hi @ocefpaf, is this PR fine?

h5netcdf
scipy
nco
numpy>=1.14,<2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to repeat anything that is already a mandatory dependency. Those are already installed when we install the package.

@ocefpaf
Copy link
Member

ocefpaf commented Jun 27, 2024

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
Copy link
Contributor Author

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
Copy link
Member

ocefpaf commented Jul 9, 2024

@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?

@Sakshamgupta90 Sakshamgupta90 force-pushed the update/requirements branch 2 times, most recently from e7cc0bf to aefcdfa Compare July 10, 2024 08:09
@ocefpaf
Copy link
Member

ocefpaf commented Jul 10, 2024

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

@Sakshamgupta90
Copy link
Contributor Author

Sakshamgupta90 commented Jul 10, 2024

@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
Copy link
Member

ocefpaf commented Jul 10, 2024

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
Copy link
Contributor Author

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
Copy link
Member

ocefpaf commented Jul 15, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Member

ocefpaf commented Jul 23, 2024

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
Copy link
Contributor Author

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
Copy link
Member

ocefpaf commented Jul 23, 2024

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
Copy link
Contributor Author

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 ocefpaf added the GSoC24 label Aug 7, 2024
@ocefpaf ocefpaf merged commit 5c974e6 into ioos:main Aug 7, 2024
12 of 13 checks passed
@Sakshamgupta90
Copy link
Contributor Author

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
Copy link
Member

ocefpaf commented Aug 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants