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

Lowess and PolyFitWithCI updates #12

Merged
merged 1 commit into from
Jul 17, 2024
Merged

Lowess and PolyFitWithCI updates #12

merged 1 commit into from
Jul 17, 2024

Conversation

Ofosu-Osei
Copy link
Owner

This is an initial push of the current update for your review. I'll deploy to pypi if no changes are needed for now.

@Ofosu-Osei Ofosu-Osei requested a review from nickeubank July 9, 2024 22:41
@@ -81,38 +79,21 @@ def test_line_label(sample_data, cleanup_files):
```python
import seaborn_objets_recipes as sor
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo there

Copy link
Owner Author

Choose a reason for hiding this comment

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

I see, thanks!

@@ -37,7 +36,7 @@ Please refer to the [testing file](https://github.com/Ofosu-Osei/seaborn_objects
```python
import seaborn_objets_recipes as sor
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

@@ -125,27 +106,23 @@ def test_lowess():
```python
import seaborn_objets_recipes as sor
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

.add(
sor.LineLabel(offset=5),
so.Agg(),
sor.Rolling(window_type="gaussian", window_kwargs={"std": 2}),
rolling,
legend=False,
)
.add(so.Band(), so.Est(errorbar="se"), legend=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see a band in the figure below — is there supposed to be one, and if not, what's this line doing?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I agree, I think I took it out in one of my actual testing but forgot it here. I'll take it out.

@nickeubank
Copy link
Collaborator

I'd suggest taking all the code snippets in the README and make sure they run — I think there are a few issues there?

@nickeubank
Copy link
Collaborator

Other than that I think we're set! I'll make an issue with next steps

@Ofosu-Osei
Copy link
Owner Author

I'd suggest taking all the code snippets in the README and make sure they run — I think there are a few issues there?

So there is a reference to a complete testing processes at the beginning of the usage section. However, I can replicate the complete code here if necessary.

@nickeubank
Copy link
Collaborator

Users -- some of whom won't have a good sense of testing -- will want to see examples so I think what's there is good -- you just wanna make sure it works

@Ofosu-Osei Ofosu-Osei merged commit e2273d5 into main Jul 17, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants