SM24-Industrial-Software-Dev / ML-forecasting-NOx-levels

0 stars 0 forks source link

Simple CBSA file ingester #13

Closed RimazeiHamachshav closed 1 hour ago

RimazeiHamachshav commented 4 days ago

Can you add some code to the demo that allows requesting a specific geometry (like NYC's)?

That will be in a separate issue, branch, and pull request.

rameshnatarajanus commented 4 days ago

This looks good. Great job.

You could use a literal type for the resolution argument to ensure only valid arguments were passed in (see [here](get_cbsa_shapefile(year=2023, resolution='5m'))). Alternatively check the function inputs for correctness, and in this case you use a default if the values are incorrect.

RimazeiHamachshav commented 4 days ago

This looks good. Great job.

You could use a literal type for the resolution argument to ensure only valid arguments were passed in (see [here](get_cbsa_shapefile(year=2023, resolution='5m'))). Alternatively check the function inputs for correctness, and in this case you use a default if the values are incorrect.

I followed your second option, although I did not handle errors caused by inputting a non-integer as a year, as that is an unlikely circumstance. Is that okay?

rameshnatarajanus commented 4 days ago

This looks good. Great job. You could use a literal type for the resolution argument to ensure only valid arguments were passed in (see [here](get_cbsa_shapefile(year=2023, resolution='5m'))). Alternatively check the function inputs for correctness, and in this case you use a default if the values are incorrect.

I followed your second option, although I did not handle errors caused by inputting a non-integer as a year, as that is an unlikely circumstance. Is that okay?

Yes that's fine. About the year, I was in 2 minds whether to leave it as it is or remove it as a function argument. It is unlikely that we will use anything but year=2023 (the latest year for which census data is available), so not sure there is any point having it as a parameter at all.

RimazeiHamachshav commented 4 days ago

This looks good. Great job. You could use a literal type for the resolution argument to ensure only valid arguments were passed in (see [here](get_cbsa_shapefile(year=2023, resolution='5m'))). Alternatively check the function inputs for correctness, and in this case you use a default if the values are incorrect.

I followed your second option, although I did not handle errors caused by inputting a non-integer as a year, as that is an unlikely circumstance. Is that okay?

Yes that's fine. About the year, I was in 2 minds whether to leave it as it is or remove it as a function argument. It is unlikely that we will use anything but year=2023 (the latest year for which census data is available), so not sure there is any point having it as a parameter at all.

At this early stage, why not keep our options open? It's your call. Can you merge it now or should I do so?

rameshnatarajanus commented 4 days ago

This looks good. Great job. You could use a literal type for the resolution argument to ensure only valid arguments were passed in (see [here](get_cbsa_shapefile(year=2023, resolution='5m'))). Alternatively check the function inputs for correctness, and in this case you use a default if the values are incorrect.

I followed your second option, although I did not handle errors caused by inputting a non-integer as a year, as that is an unlikely circumstance. Is that okay?

Yes that's fine. About the year, I was in 2 minds whether to leave it as it is or remove it as a function argument. It is unlikely that we will use anything but year=2023 (the latest year for which census data is available), so not sure there is any point having it as a parameter at all.

At this early stage, why not keep our options open? It's your call. Can you merge it now or should I do so?

  1. The defaults for year dont seem right. The census only supports a fixed set of dates [ 2014, 2017, ... ] etc. which are the years the census was performed. So restrict the years to that and the default can be the most recent one 2023.
  2. Re your type question on year you can use type annotations to handle that, see here. In fact strongly suggest doing this always both in your code as well as in the code reviews.
RimazeiHamachshav commented 2 days ago

This looks good. Great job. You could use a literal type for the resolution argument to ensure only valid arguments were passed in (see [here](get_cbsa_shapefile(year=2023, resolution='5m'))). Alternatively check the function inputs for correctness, and in this case you use a default if the values are incorrect.

I followed your second option, although I did not handle errors caused by inputting a non-integer as a year, as that is an unlikely circumstance. Is that okay?

Yes that's fine. About the year, I was in 2 minds whether to leave it as it is or remove it as a function argument. It is unlikely that we will use anything but year=2023 (the latest year for which census data is available), so not sure there is any point having it as a parameter at all.

At this early stage, why not keep our options open? It's your call. Can you merge it now or should I do so?

  1. The defaults for year don't seem right. The census only supports a fixed set of dates [ 2014, 2017, ... ] etc. which are the years the census was performed. So restrict the years to that and the default can be the most recent one 2023.
  2. Re your type question on year you can use type annotations to handle that, see here. In fact strongly suggest doing this always both in your code as well as in the code reviews.

Re: 1. - The Census Bureau has files for all years between 2014-2022, inclusive, in the same filepath as the 2023 files. Re: 2. - I added type hints. However, type hints don't do anything at runtime, and my concern was handling invalid inputs. Unless your response meant to add something besides type hints?