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

fix clustering bug due to missing countries in n.buses #861

Merged
merged 4 commits into from
Jan 18, 2024
Merged

Conversation

p-glaum
Copy link
Contributor

@p-glaum p-glaum commented Jan 12, 2024

Changes proposed in this Pull Request

filter looking for empty values in n.buses.countries is not working anymore. This leads to errors later on in the cluster_network script. for 37 nodes, the error in the cluster script was that there are more combinations of subnetworks and countries than 37 and for higher cluster numbers this lead to a problem due to a division by 0.

The CI did not catch the mistake in the current master, because we only consider one country for the test CI

Checklist

  • I tested my contribution locally and it seems to work fine.
  • Code and workflow changes are sufficiently documented.
  • Changed dependencies are added to envs/environment.yaml.
  • Changes in configuration options are added in all of config.default.yaml.
  • Changes in configuration options are also documented in doc/configtables/*.csv.
  • A release note doc/release_notes.rst is added.

@martacki
Copy link
Member

I just have changed this due to the new pandas version, where .isna() was not working anymore. Which pandas version are you using?

@p-glaum
Copy link
Contributor Author

p-glaum commented Jan 15, 2024

I am now also working on the newest pandas version and there isna() is working but it does not filter as I intended it to do. Is the implementation with == "na" working for you?

@p-glaum
Copy link
Contributor Author

p-glaum commented Jan 15, 2024

I think to avoid any problems with pandas, we could just do
buses.loc[:, "country"] = ""
and then later filter for
c_nan_b = buses.country == ""

@FabianHofmann
Copy link
Contributor

I think to avoid any problems with pandas, we could just do buses.loc[:, "country"] = "" and then later filter for c_nan_b = buses.country == ""

I agree, but since the country column is of dtype string, we should rather track down where the nans come from and make sure we use "" instead. Then, the filter would be n.buses.country == ""

@p-glaum
Copy link
Contributor Author

p-glaum commented Jan 15, 2024

The nan values came from a previous pandas version. In the latest version, those entries are just empty. But we somehow need to filter them and neither isna() nor isnull() are working .

@p-glaum
Copy link
Contributor Author

p-glaum commented Jan 15, 2024

So basically with the newest pandas version =="na" is working as Martha did it, so I can close this PR. If pandas should decide to change again how missing values are treated, we could use something as I suggested above.

@p-glaum p-glaum closed this Jan 15, 2024
@fneum fneum reopened this Jan 15, 2024
@fneum
Copy link
Member

fneum commented Jan 15, 2024

If it only works for a particular pandas version, this should be reflected in the environment.yaml. Ideally, the minimum version should not be the very latest pandas version. We might need a workaround that works for more versions.

@p-glaum
Copy link
Contributor Author

p-glaum commented Jan 15, 2024

The workaround could be the one I suggested. I.e. do buses.loc[:, "country"] = ""
and then later filter for c_nan_b = buses.country == ""

@FabianHofmann
Copy link
Contributor

or just add a .fillna("na") before the comparison

Co-authored-by: Fabian Hofmann <fab.hof@gmx.de>
@fneum fneum enabled auto-merge January 18, 2024 17:22
@fneum fneum disabled auto-merge January 18, 2024 18:34
@fneum fneum merged commit 85ecb13 into master Jan 18, 2024
5 checks passed
@fneum fneum deleted the fix_clustering branch January 18, 2024 18:35
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.

4 participants