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 make_test_grib_idx function and tests #485

Closed
wants to merge 5 commits into from

Conversation

Anu-Ra-g
Copy link
Contributor

No description provided.

@martindurant
Copy link
Member

For local files, I recommend you use full absolute paths to avoid confusion. Also, if you want to test how your function works (which looks like a fixture, by the way), you can first copy the file to local rather than have two filesystems to get confused between.

@Anu-Ra-g
Copy link
Contributor Author

I thought make_test_grib_idx_files function was part of dynamic zarr store module so I added it. Should we add this to the kerchunk repo?

@martindurant
Copy link
Member

I don't understand - this PR is already wanting to add that function to the kerchunk repo (this repo).

@martindurant
Copy link
Member

If it isn't clear, my original comment above was in response to

I need your help here. I can't figure how this test is failing. I've tried two way but failing on both.

which request of yours since disappeared.

@Anu-Ra-g
Copy link
Contributor Author

No, I meant you said this function looks like a fixture, so I asked the question.

@martindurant
Copy link
Member

Yes, the function could be made into a function and included in this repo. It is only used for tests, right?

@Anu-Ra-g
Copy link
Contributor Author

Yes, it will be used for tests.

@martindurant
Copy link
Member

Will it be used ONLY for tests? If yes, you can try making it a @pytest.fixture. It's purely a style thing, it seems like a fixture to me.

@Anu-Ra-g
Copy link
Contributor Author

Anu-Ra-g commented Jul 29, 2024

Also, if you want to test how your function works (which looks like a fixture, by the way), you can first copy the file to local rather than have two filesystems to get confused between.

The original implementation copied subset of the grib file to the cloud. I modified it so the subset file could be locally or to the cloud (if you have access to it).

@martindurant
Copy link
Member

so the subset file could be locally or to the cloud

Right, but apparently this wasn't working as you expected. Thus, it might be simpler to first copy a given file to local, and then make its .idx local->local.

@Anu-Ra-g
Copy link
Contributor Author

I meant test wasn't working. The function work fine.

@martindurant
Copy link
Member

If the test fails, maybe you can't be certain that the function is working fine.

@Anu-Ra-g
Copy link
Contributor Author

Anu-Ra-g commented Jul 29, 2024

No I meant the test which I wrote for the make_grib_test_files function is failing.

@Anu-Ra-g
Copy link
Contributor Author

Also this function isn't used anywhere in the actual flow or in the tests. There was bunch of grib-idx pairs the in the asascience-open/nextgen-dmac repository made by David. I waiting for his reply about what to do with this function.

@martindurant
Copy link
Member

Also this function isn't used anywhere in the actual flow or in the tests.

??
Then why are you wanting to add it to the repo at all?

@Anu-Ra-g
Copy link
Contributor Author

It was in the original repo, so I thought of adding it. I'll close this PR.

@Anu-Ra-g Anu-Ra-g closed this Jul 29, 2024
@Anu-Ra-g Anu-Ra-g deleted the make_grib branch August 2, 2024 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants