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 typo in design_landscape vignette #68

Closed
wants to merge 1 commit into from

Conversation

mmore500
Copy link
Contributor

@mmore500 mmore500 commented Dec 1, 2023

Appears to have been introduced by an erroneous renaming? 130bc18?diff=unified&w=0#r134004700

@benj919
Copy link
Collaborator

benj919 commented Dec 5, 2023

Hi @mmore500
Thank you for this pull request.

I agree there's an obvious error here:

temp_dataframe[counting,2+timestep]<-temp_dataframe[x,y,timestep]
Error in temp_dataframe[x, y, timestep] : incorrect number of dimensions

Though running your fix does not solve that:

temp_dataframe[counting,2+timestep]<-temp_matrix[x,y,timestep]
Error: object 'temp_matrix' not found

I think the fixes you are looking for are:

temp_dataframe[counting,2+timestep]<-temp_array[x,y,timestep]
prec_dataframe[counting,2+timestep]<-prec_array[x,y,timestep]

Though to be honest I think the whole thing could be done in one pass without copying the values around 🤷‍♂️

I'll close this pull request,thanks again for taking the time to look into this and open the pull request. Please let us know if you intend to open a new one with a fix?

cheers

@benj919 benj919 closed this Dec 5, 2023
@mmore500
Copy link
Contributor Author

mmore500 commented Dec 5, 2023 via email

@benj919
Copy link
Collaborator

benj919 commented Dec 8, 2023

That would work, yes. If you do so please do a quick check (without adding that to the pull request) and plot the resulting rasters on line 136/137 to make sure that there's actually something put into the rasters.

Thanks again for your efforts 👍

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