Skip to content

Conversation

ThomasGl
Copy link
Contributor

@ThomasGl ThomasGl commented Sep 4, 2024

This is a solution to the error I made at the Issue No. 200

Such fix allows us to run the following code:

import pandas as pd
import datetime as dt
import numpy as np
df = pd.DataFrame([
    {"ref_date": dt.date(2024, 1, 1), "data": 1},
    {"ref_date": dt.date(2024, 1, 2), "data": 4},
    {"ref_date": dt.date(2024, 1, 3), "data": None},
    {"ref_date": dt.date(2024, 1, 4), "data": 2},
    {"ref_date": dt.date(2024, 1, 5), "data": 4},
    {"ref_date": dt.date(2024, 1, 6), "data": None},
])

df['ref_date'] = pd.to_datetime(df['ref_date'])
df.set_index('ref_date', inplace=True)
df.index = (df.index.astype(np.int64) / 10**6).astype(np.int64)

from highcharts_core.chart import Chart
chart = Chart.from_pandas(
    df=df.reset_index(),
    series_type='line',
    property_map={
        'x': df.index.name,
        'y': df.columns.to_list()
    }
)

chart.options.x_axis = {
    'type': 'datetime'
}

chart.display()

And get:

image

Under the hood it's swapping nan for null, which is a valid None under javascript.

@hcpchris hcpchris self-requested a review September 5, 2024 00:11
@hcpchris hcpchris changed the base branch from master to develop September 5, 2024 00:12
Copy link
Collaborator

@hcpchris hcpchris left a comment

Choose a reason for hiding this comment

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

Thanks, @ThomasGl ! #200 is a great catch, and this PR is a great solution. I've got one minor suggestion that I'd recommend which I've commented on the specific line. Thanks for the PR! If you make the adjustment, I'll run the CI/CD tests and incorporate the PR into a patch fix for Highcharts Core for Python.

@ThomasGl ThomasGl force-pushed the hotfix/jupyter/nan-handler branch from fefe11c to 6f2b045 Compare September 6, 2024 02:19
@ThomasGl
Copy link
Contributor Author

ThomasGl commented Sep 6, 2024

@hcpchris I have adjusted the PR with a an amend to my commit, thank you for the suggestion. I tested locally on my machine and it does indeed to do the trick to keep numpy as a soft dependency

@ThomasGl
Copy link
Contributor Author

ThomasGl commented Sep 6, 2024

Hi Chris, it seems there's some safety block preventing the build to pass successfully, let me know if you have any tips on how to correctly adjust this part. Thanks

@ThomasGl
Copy link
Contributor Author

ThomasGl commented Sep 9, 2024

Hi Chris, let me know if you found something that could assist me.

Best regards,
Thomas

@hcpchris
Copy link
Collaborator

hcpchris commented Sep 9, 2024

Hi @ThomasGl : I looked into it, and the issue is tied to how NumPy casts (converts) objects of different types into a single type when evaluating them in their "universal functions" (of which np.isnan() is one).

By default, NumPy applies safe casting, which ensures that there is no loss of information when values are cast (the canonical example would be that an int could be cast to a float, but a float could not be cast to an int because that risks losing information).

The CI/CD errors are occurring because the safe casting is determining that it cannot cast a dict without risking loss of data. Which is correct / valid behavior.

The fix I would recommend would be to adjust the conditional elif HAS_NUMPY and np.isnan(item): line. In particular, I'd add an additional condition like the below:

...
elif HAS_NUMPY and not isinstance(item, (dict, UserDict)) and np.isnan(item):
...

This should allow NumPy to still apply safe casting, and ensure that dict values get handled by the appropriate get_js_literal() condition (which gets triggered a little later down in the codebase). Granted, this may not work - there may be something else that trips us up, but this is what I would recommend trying as the first / simplest solution.

@ThomasGl ThomasGl force-pushed the hotfix/jupyter/nan-handler branch from 6f2b045 to d8004e0 Compare September 9, 2024 15:23
@ThomasGl
Copy link
Contributor Author

ThomasGl commented Sep 9, 2024

Hi Chris, looks like it worked indeed. Build was successfull. Let me know if there's any extra steps from my part!

@ThomasGl
Copy link
Contributor Author

ThomasGl commented Sep 9, 2024

Correction: the build didn't work. It crashed with the same errors as before, 'safe' casting

@ThomasGl
Copy link
Contributor Author

Hi Chris hows it going? Do you have any other guesses on how to address the build?

@ThomasGl
Copy link
Contributor Author

Hey chris, is there any follow up on this PR?

@hcpchris hcpchris changed the base branch from develop to bugfix/200-nan-handling-bugfix September 23, 2024 14:49
@hcpchris
Copy link
Collaborator

Hi @ThomasGl - Sorry for the delay on this. I've been trying to find a better generic solution since np.isnan() errors out with a TypeError when using safe casting for certain data types, which is causing the fix to still fail. I'm testing all of the expected data types to determine which error types are causing that failure, which may affect how to best handle the situation. I see a couple potential solutions, but which one works best depends on which data types cause np.isnan() to throw the TypeError.

@hcpchris
Copy link
Collaborator

@ThomasGl : Okay, I seem to have resolved the issue. I did the following:

  1. I created a new branch in Highcharts Core bugfix/200-nan-handling-bugfix.
  2. I changed the "base" of this PR to instead point at that branch.
  3. In that branch, I added a set of unit tests to both isolate the patterns where np.isnan() throws various errors, and
  4. In that branch, I adjusted the code in get_js_literal() to handle circumstances where TypeError was getting thrown invalidly.
  5. I updated your branch with the relevant adjustments.

Given this, I'm going to close this PR (without merging your now out-dated changes), and merge the bugfix/200-nan-handling-bugfix branch into a release candidate (and subsequently release it) with the fix. Thanks for your help identifying and resolving this issue! I'll add you to the contributor list along with the release.

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