deephealthproject / eddl

European Distributed Deep Learning (EDDL) library. A general-purpose library initially developed to cover deep learning needs in healthcare use cases within the DeepHealth project.
https://deephealthproject.github.io/eddl/
MIT License
34 stars 10 forks source link

`get_fmem()` issues #244

Closed MicheleCancilla closed 3 years ago

MicheleCancilla commented 3 years ago

I was looking for windows incompatibilites in develop branch and I spotted some issues regarding the get_fmem() function that could be solved.

-

  unsigned long freemem = get_free_mem();

The get_free_mem() function returns an unsigned long which could cause integer overflow. I would move return type to unsigned long long.

I can open a PR so you can test the changes.

jonandergomez commented 3 years ago

Thanks Michele, I'm going to solve it asap.

jonandergomez commented 3 years ago

Dear Michele,

I just pushed a commit into develop facing what you commented, but I have no the chance to test it in Windows, could you check it? I'm ready to fix any bug derived from this commit.

Regads,

Jon

MicheleCancilla commented 3 years ago

Dear Jon, the tests do not pass on windows, but I understand why. The TensorTestSuite.tensor_math_unary_log test fails throwing exception in eddl_malloc. I have verified that errno != 0 is true before calling _aligned_malloc and errno value is 33 (EDOM Math argument).

auto err_ = errno;
ptr = _aligned_malloc(size, alignment_block_size);
error = (nullptr == ptr || errno != 0);
err_ = errno;

I suggest only checking if allocation fails (doc):

ptr = _aligned_malloc(size, alignment_block_size);
error = (nullptr == ptr || errno == ENOMEM);



I get a different compilation error (a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax) on _5_nlp_textgeneration. You should remove parentheses around vtensor:

forward(decoder, vtensor{ word,treshape,state });
CostantinoGrana commented 3 years ago

For everybody: errno should be set to 0 before calling a function, if you plan to check its value. errno should be checked only in case of error in function. Check this: https://softwareengineering.stackexchange.com/a/209731/360023

jonandergomez commented 3 years ago

Thanks for the comment Costantino. And thanks for the suggestion Michele.

Let me solve it and I will push a commit.

Jon

jonandergomez commented 3 years ago

Hi again,

Commit following your indications pushed, also the compiling problem with the call to forward in the NLP example fixed.

Michele, could you please check in windows if both problems are solved?

Regards,

Jon

MicheleCancilla commented 3 years ago

No, it does not work 😂 There is an error in the errno condition error = (nullptr == ptr || errno != ENOMEM);. It should be error = (nullptr == ptr || errno == ENOMEM);.

jonandergomez commented 3 years ago

Sorry, my logic fails. I need a windows instance to do checks and avoid bothering you with this. Please, check it after the last commit and close the issue if finally it runs. Regards, Jon

MicheleCancilla commented 3 years ago

Yes, it works! Thank you Jon.