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

WIP: Address Crystal 0.36.1 complaints and latest alea update #64

Closed
wants to merge 1 commit into from

Conversation

lbarasti
Copy link

@lbarasti lbarasti commented Mar 1, 2021

Hi folks, this is to start the conversation on what needs to be done to ensure num.cr works with the latest version of alea (which now requires Crystal v0.36.0 or greater).

See #63

@@ -34,5 +34,5 @@ abstract class Num::Grad::Gate(T)
abstract def backward(payload : Num::Grad::Payload(T)) : Array(T)

# Caches the result of an operation on a context
abstract def cache(result : Num::Grad::Variable(T), *args)
abstract def cache(result : Num::Grad::Variable(T), *args : Num::Grad::Variable(T))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the issue with this that child methods sometimes say args is an instance of Num::Grad::Variable, which isn't explicitly defined by the abstract base, or that args must now have a defined type?

cache methods can take anything that a gate needs to store information. For example, the convolutional gate's cache method also takes tuples of integers for padding and stride.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Chris, the issue is the compiler now complains that the implementations are too strict

In src/grad/gates_blas.cr:25:1

 25 | class Num::Grad::MatMulGate(T) < Num::Grad::Gate(T)
      ^
Error: abstract `def Num::Grad::Gate(T)#cache(result : Num::Grad::Variable(T), *args)` must be implemented by Num::Grad::MatMulGate(T)

So either we restrict the abstract def or we remove the type restriction on each def cache definition

image

@christopherzimmerman
Copy link
Member

Thank you for the PR. I haven't had time to work much on this library recently, but I will double check tomorrow to make sure everything works with the most recent version.

@christopherzimmerman
Copy link
Member

@lbarasti closing this in favor of the now merged #65. I still need to do some work with library dependencies, to make transitions smoother.

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.

None yet

2 participants