search

OpenDrift / opendrift

Open source framework for ocean trajectory modelling
https://opendrift.github.io
GNU General Public License v2.0
231 stars 113 forks source link

Bug in ROMS native reader #1281

Closed ivicajan closed 2 months ago

ivicajan commented 2 months ago

Hi, think you should add "angle" as a variable you want to keep when using mfdataset (multiple files). Otherwise, you can't rotate roms currents. This does not affect a single file read. Basically, in line 156 of opendrift/readers/reader_ROMS_native.py should add "angle" among other variables (Vtransform etc).

Cheers, Ivica

knutfrode commented 2 months ago

Hi,

Kristin Thyng recently made a method angle which dynamically loads the angle whenever needed: https://github.com/OpenDrift/opendrift/blob/master/opendrift/readers/reader_ROMS_native.py#L376

This seems to work well, as all tests are passing. Are you getting any problems related to this?

ivicajan commented 2 months ago

Hi Knut, if you look at the 156 line then you'll see that you are keeping just a subset of static variables and there is no angle. This is using Xarray drop_variables option and is relevant only when you open multiple files via Xarray.

I do have a problem if I load multiple roms files in a folder (i.e. roms = reader_ROMS_native.Reader('roms_his_204*.nc')

In case I modify line 156 from the original: 'Cs_r', 'hc', 'Vtransform'] into: 'Cs_r', 'hc', 'Vtransform', 'angle']

it is working OK.

Cheers, Ivica

MoraneC commented 2 months ago

Hi all, @ivicajan @knutfrode, I had the same issue with multiple roms files. When adding 'angle', at l.156 in reader_ROMS_native, as you did, it worked for me as well.

Best,

Morane

knutfrode commented 2 months ago

I do not have ROMS native files readily available for testing, but trust you are right.

angle was removed from the opened Dataset in this commit: https://github.com/OpenDrift/opendrift/commit/006cee6f53c966999d9e195c4ae46e4848ae0bdd @kthyng It is ok to add back angle on line 156, or would that interfere with anything else? Have you tried the ROMS reader yourself with multiple files (e.g. wildcards in filename)?

ivicajan commented 2 months ago

Hi Knut, yes, it is safe to add suggested change. Tried with multiple files (*.nc) when it is invoking xr.open_mfdataset and using drop_variables option.

Cheers,

Ivica

From: Knut-Frode Dagestad @.> Sent: Friday, April 19, 2024 6:05:34 PM To: OpenDrift/opendrift @.> Cc: Ivica Janekovic @.>; Mention @.> Subject: Re: [OpenDrift/opendrift] Bug in ROMS native reader (Issue #1281)

I do not have ROMS native files readily available for testing, but trust you are right.

angle was removed from the opened Dataset in this commit: 006cee6https://github.com/OpenDrift/opendrift/commit/006cee6f53c966999d9e195c4ae46e4848ae0bdd @kthynghttps://github.com/kthyng It is ok to add back angle on line 156, or would that interfere with anything else? Have you tried the ROMS reader yourself with multiple files (e.g. wildcards in filename)?

— Reply to this email directly, view it on GitHubhttps://github.com/OpenDrift/opendrift/issues/1281#issuecomment-2066249361, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AEN4TMFVO33JJBB657IJPLTY6DT65AVCNFSM6AAAAABGMPEFC6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRWGI2DSMZWGE. You are receiving this because you were mentioned.Message ID: @.***>

kthyng commented 2 months ago

@knutfrode I am pretty sure my intention in removing angle from that list was to keep it from being dropped, so that was an accident. Sorry about messing that up!

knutfrode commented 2 months ago

No problem, but then it makes all sense. I have now corrected this in master: https://github.com/OpenDrift/opendrift/pull/1284/files Thus closing this issue.