search

UW-Hydro / VIC

The Variable Infiltration Capacity (VIC) Macroscale Hydrologic Model
http://vic.readthedocs.io
MIT License
262 stars 392 forks source link

Remove DIST_PREC option #22

Closed jhamman closed 10 years ago

jhamman commented 11 years ago

Description - Remove all loops over dist and dist array dimensions from code. Optional? - No

bartnijssen commented 11 years ago

Bart will help out with this as well.

bartnijssen commented 10 years ago

When removing the DIST_PRCP option we could just get rid of dist_prcp.c and invoke full_energy(), put_data(), and write_model_state() directly from vicNl.c, which would make it easier to read.

tbohn commented 10 years ago

Yes, I agree. The hard part is in removing all of those loops over the "dist" dimension throughout all the mid-level functions.

tbohn commented 10 years ago

In making this change, one thing I could potentially do is rename the "dist_prcp" structure to something that doesn't reference the DIST_PREC feature. Ideally, I would think something like "cell" would be most appropriate (and renaming the current "cell" structure to "soil"), since the dist_prcp structure is the top-level container for all states/fluxes of a given cell. However, this would require making tons of changes throughout the code - with opportunities for errors along the way.

Perhaps I should rename it to something else, like "column", or just not rename it at all. Any suggestions?

bartnijssen commented 10 years ago

Maybe a first step would be a little graphic that shows the dependencies between the different data structures. That will probably quickly point us to the right way of getting rid of DIST_PREC (or renaming it).

I agree that renaming it to the name of an existing structure maybe asking for errors. What does the original CELL structure refer to (this should be easy to see from the diagram).

tbohn commented 10 years ago

Just an update on my progress. I've removed the dist_prec() function, moving the calls to full_energy(), put_data(), and write_model_state() into main(). I've removed all variables related to the DIST_PRCP option.

I'm in the process of renaming the "dist_prcp" structure to "all_data", since it contains all other data structures (except for atmos) that describe the states/fluxes of the cell control volume.

The final step will be to remove all loops over "dist" and reduce the dimensions of cell_data and veg_var from 3 to 2.

jhamman commented 10 years ago

I'm reopening this since I'm seeing a few relic dist_precip references. For example:

These were likely merge issues on my part but they should be cleaned up before we close this issue.

jhamman commented 10 years ago

I just noticed that initialize_new_storm.c is still there too...

tbohn commented 10 years ago

Hi guys, are you waiting on me to fix this (it's assigned to me)? Perhaps it would be more appropriate to reassign to Bart, since he did the port from 4.2? Or, am I missing something?

bartnijssen commented 10 years ago

Not entirely clear to me either. I am pretty sure it's all gone from the version I am working on (though need to check). I'd assign it to Joe for the time being since he alludes to a merge error.

On Sep 24, 2014, at 7:19 PM, Ted Bohn notifications@github.com wrote:

Hi guys, are you waiting on me to fix this (it's assigned to me)? Perhaps it would be more appropriate to reassign to Bart, since he did the port from 4.2? Or, am I missing something?

— Reply to this email directly or view it on GitHub.